[lustre-devel] [PATCH 24/30] lustre: llite: rcu-walk check should not depend on statahead

[NeilBrown]

On Sat, Sep 29 2018, James Simmons wrote:

>> On Mon, Sep 17 2018, James Simmons wrote:
>> 
>> > From: Steve Guminski <stephenx.guminski at intel.com>
>> >
>> > Moves the check for the LOOKUP_RCU flag, so that it does not depend
>> > on the statahead setting.  The caller is now informed if rcu-walk
>> > was requested but the filesystem does not support it, regardless
>> > of whether statahead is enabled or disabled.
>> 
>> Nope, this is wrong.
>> 
>> The filesystem only returns -ECHILD if it couldn't complete the request
>> because it would have to block.
>> If statahead is disabled, then it can complete the request immediately,
>> doesn't need to block, and so doesn't need to return -ECHILD.
>> 
>> Patch deleted.
>
> Do this patch need to be reverted in the OpenSFS branch or can it be
> ignored?

It can be ignored.
The justification for the patch (and it is definitely nice to see a
clearly stated justification!!) was based on a misunderstanding, and as
wrong.
The code itself introduces a small performance hit when statahead is not
being used.  I can't really say if it would be noticeable or not, but
you could only notice it on a many CPU machine where lots of the
filesystem was cached locally, and there was no need to go to the server
to service lots of lookups.

NeilBrown


>
>> > Signed-off-by: Steve Guminski <stephenx.guminski at intel.com>
>> > WC-bug-id: https://jira.whamcloud.com/browse/LU-8891
>> > Reviewed-on: https://review.whamcloud.com/24195
>> > Reviewed-by: John L. Hammond <jhammond at whamcloud.com>
>> > Reviewed-by: Andreas Dilger <adilger at whamcloud.com>
>> > Reviewed-by: Oleg Drokin <green at whamcloud.com>
>> > Signed-off-by: James Simmons <jsimmons at infradead.org>
>> > ---
>> >  drivers/staging/lustre/lustre/llite/dcache.c | 7 +++----
>> >  1 file changed, 3 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/staging/lustre/lustre/llite/dcache.c b/drivers/staging/lustre/lustre/llite/dcache.c
>> > index 11b82c63..ee1ba16 100644
>> > --- a/drivers/staging/lustre/lustre/llite/dcache.c
>> > +++ b/drivers/staging/lustre/lustre/llite/dcache.c
>> > @@ -270,13 +270,12 @@ static int ll_revalidate_dentry(struct dentry *dentry,
>> >  	if (lookup_flags & LOOKUP_REVAL)
>> >  		return 0;
>> >  
>> > -	if (!dentry_may_statahead(dir, dentry))
>> > -		return 1;
>> > -
>> >  	if (lookup_flags & LOOKUP_RCU)
>> >  		return -ECHILD;
>> >  
>> > -	ll_statahead(dir, &dentry, !d_inode(dentry));
>> > +	if (dentry_may_statahead(dir, dentry))
>> > +		ll_statahead(dir, &dentry, !d_inode(dentry));
>> > +
>> >  	return 1;
>> >  }
>> >  
>> > -- 
>> > 1.8.3.1
>> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20181002/cf2a0e2e/attachment.sig>