| Message ID | 1537205440-6656-25-git-send-email-jsimmons@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lustre: first batch of fixes from lustre 2.10 | expand |
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
> 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 >
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 >>
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; }