On Jul 5, 2016, at 4:08 PM, Al Viro wrote:
> On Tue, Jul 05, 2016 at 03:12:44PM -0400, Oleg Drokin wrote:
>>
>> When d_in_lookup was introduced, it was described as:
>> New primitives: d_in_lookup() (a predicate checking if dentry is in
>> the in-lookup state) and d_lookup_done() (tells the system that
>> we are done with lookup and if it's still marked as in-lookup, it
>> should cease to be such).
>>
>> I don't see where it mentions anything about exclusive vs parallel lookup
>> that probably led to some confusion.
>
> In the same commit:
>
> #define DCACHE_PAR_LOOKUP 0x10000000 /* being looked up (with
> parent locked shared) */
Well, I did not really check the commit, just the log message,
since there's no other documentation about it apparently ;)
>> So with Lustre the dentry can be in three states, really:
>>
>> 1. hashed dentry that's all A-ok to reuse.
>> 2. hashed dentry that's NOT valid (dlm lock went away) - this is
>> distinguished in d_compare by looking at a bit in the fs_data
>> 3. unhashed dentry ( I guess could be both valid and invalid lustre-wise).
>>
>> So the logic in ll_lookup_it_finish() (part of regular lookup) is this:
>>
>> If the dentry we have is not hashed - this is a new lookup, so we need to
>> call into ll_splice_alias() to see if there's a better dentry we need to
>> reuse that was already rejected by VFS before since we did not have
>> necessary locks,
>> but we do have them now.
>> The comment at the top of ll_dcompare() explains why we don't just unhash the
>> dentry on lock-loss - that apparently leads to a loop around real_lookup for
>> real-contended dentries.
>> This is also why we cannot use d_splice_alias here - such cases are possible
>> for regular files and directories.
>>
>> Anyway, I guess additional point of confusion here is then why does
>> ll_lookup_it_finish() need to check for hashedness of the dentry since it's
>> in
>> lookup, so we should be unhashed here.
>> I checked the commit history and this check was added along with atomic_open
>> support, so I imagine we can just move it up into ll_atomic_open and then
>> your
>> change starts to make sense along with a few other things.
>
> So basically this
> } else if (!it_disposition(it, DISP_LOOKUP_NEG) &&
> !it_disposition(it, DISP_OPEN_CREATE)) {
> /* With DISP_OPEN_CREATE dentry will be
> * instantiated in ll_create_it.
> */
> LASSERT(!d_inode(*de));
> d_instantiate(*de, inode);
> }
> is something we should do only for negative hashed fed to it by
> ->atomic_open(), and that - only if we have no O_CREAT in flags?
Yes, and in fact we can totally avoid it I think.
> Then, since 3/3 eliminates that case completely, we could just rip that
> else-if, along with the check for d_unhashed itself, making the call of
> ll_splice_alias() unconditional there. Or am I misreading what you said?
> Do you see any problems with what's in #for-linus now (head at 11f0083)?
Yes, we can make it unconditional
I think we can simplify it even more and since unlike NFS our negative dentries
are a lot less speculative, we can just return ENOENT if this is not a create
request and unhash otherwise. Still needs to go through the whole test suite
to make sure it does not break anything unexpected.
At least this is the patch I am playing with right now:
--- a/drivers/staging/lustre/lustre/llite/namei.c
+++ b/drivers/staging/lustre/lustre/llite/namei.c
@@ -391,6 +391,7 @@ static int ll_lookup_it_finish(struct ptlrpc_request
*request,
struct inode *inode = NULL;
__u64 bits = 0;
int rc = 0;
+ struct dentry *alias;
/* NB 1 request reference will be taken away by ll_intent_lock()
* when I return
@@ -415,26 +416,12 @@ static int ll_lookup_it_finish(struct ptlrpc_request
*request,
*/
}
- /* Only hash *de if it is unhashed (new dentry).
- * Atoimc_open may passing hashed dentries for open.
- */
- if (d_unhashed(*de)) {
- struct dentry *alias;
-
- alias = ll_splice_alias(inode, *de);
- if (IS_ERR(alias)) {
- rc = PTR_ERR(alias);
- goto out;
- }
- *de = alias;
- } else if (!it_disposition(it, DISP_LOOKUP_NEG) &&
- !it_disposition(it, DISP_OPEN_CREATE)) {
- /* With DISP_OPEN_CREATE dentry will be
- * instantiated in ll_create_it.
- */
- LASSERT(!d_inode(*de));
- d_instantiate(*de, inode);
+ alias = ll_splice_alias(inode, *de);
+ if (IS_ERR(alias)) {
+ rc = PTR_ERR(alias);
+ goto out;
}
+ *de = alias;
if (!it_disposition(it, DISP_LOOKUP_NEG)) {
/* we have lookup look - unhide dentry */
@@ -590,6 +577,24 @@ static int ll_atomic_open(struct inode *dir, struct dentry
*dentry,
dentry, PFID(ll_inode2fid(dir)), dir, file, open_flags, mode,
*opened);
+ /* Only negative dentries enter here */
+ LASSERT(!d_inode(dentry));
+
+ if (!(open_flags & O_CREAT) && !d_in_lookup(dentry)) {
+ /* A valid negative dentry that just passed revalidation,
+ * there's little point to try and open it server-side,
+ * even though there's a minuscle chance it might succeed.
+ * Either way it's a valid race to just return -ENOENT here.
+ */
+ if (!(open_flags & O_CREAT))
+ return -ENOENT;
+
+ /* Otherwise we just unhash it to be rehashed afresh via
+ * lookup if necessary
+ */
+ d_drop(dentry);
+ }
+
it = kzalloc(sizeof(*it), GFP_NOFS);
if (!it)
return -ENOMEM;
