On Jul 5, 2016, at 4:21 PM, Oleg Drokin wrote:
>
>>> 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)) {
Duh, this obviously was supposed to be
if (!d_in_lookup(dentry)) {
But even in the form above nothing bad happened in the full testing
because we cannot find any aliases without an inode.
> + /* 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);
So we can even drop this part and retain the top condition as it was.
d_add does not care if the dentry we are feeding it was hashed or not,
so do you see any downsides to doing that I wonder?
> + }
> +
> it = kzalloc(sizeof(*it), GFP_NOFS);
> if (!it)
> return -ENOMEM;
>
>
