[24/30] lustre: llite: rcu-walk check should not depend on statahead
diff mbox series

Message ID 1537205440-6656-25-git-send-email-jsimmons@infradead.org
State New
Headers show
Series
  • lustre: first batch of fixes from lustre 2.10
Related show

Commit Message

James Simmons Sept. 17, 2018, 5:30 p.m. UTC
From: Steve Guminski <stephenx.guminski@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.

Signed-off-by: Steve Guminski <stephenx.guminski@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@whamcloud.com>
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>
Signed-off-by: James Simmons <jsimmons@infradead.org>
---
 drivers/staging/lustre/lustre/llite/dcache.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

NeilBrown Sept. 24, 2018, 4:22 a.m. UTC | #1
On Mon, Sep 17 2018, James Simmons wrote:

> From: Steve Guminski <stephenx.guminski@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.

Thanks,
NeilBrown

>
> Signed-off-by: Steve Guminski <stephenx.guminski@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@whamcloud.com>
> Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
> Reviewed-by: Oleg Drokin <green@whamcloud.com>
> Signed-off-by: James Simmons <jsimmons@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
James Simmons Sept. 29, 2018, 9:33 p.m. UTC | #2
> On Mon, Sep 17 2018, James Simmons wrote:
> 
> > From: Steve Guminski <stephenx.guminski@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?

> > Signed-off-by: Steve Guminski <stephenx.guminski@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@whamcloud.com>
> > Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
> > Reviewed-by: Oleg Drokin <green@whamcloud.com>
> > Signed-off-by: James Simmons <jsimmons@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
>
NeilBrown Oct. 2, 2018, 4:35 a.m. UTC | #3
On Sat, Sep 29 2018, James Simmons wrote:

>> On Mon, Sep 17 2018, James Simmons wrote:
>> 
>> > From: Steve Guminski <stephenx.guminski@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@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@whamcloud.com>
>> > Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
>> > Reviewed-by: Oleg Drokin <green@whamcloud.com>
>> > Signed-off-by: James Simmons <jsimmons@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
>>

Patch
diff mbox series

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;
 }