| Message ID | 20210205144248.13508-1-aaptel@suse.com |
|---|---|
| State | Accepted |
| Commit | 21b200d091826a83aafc95d847139b2b0582f6d1 |
| Headers | show |
| Series | [v4] cifs: report error instead of invalid when revalidating a dentry fails | expand |
On Fri, Feb 5, 2021 at 8:52 AM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > Looks good to me. We do need to find out how a single client test could generate ESTALE though (perhaps we forgot to drop a dentry on delete or rmdir). The test is doing fsstress, so should be easy enough to shorten it and repro and find out exactly why the ESTALE is coming. > Maybe change the FYI in the cifs_dbg line above to VFS? Probably safer to put a dynamic trace point there in case EACCES or other rc becomes too common in some scenario. > On Fri, Feb 5, 2021 at 6:42 AM Aurélien Aptel <aaptel@suse.com> wrote: > > > > From: Aurelien Aptel <aaptel@suse.com> > > > > Assuming > > - //HOST/a is mounted on /mnt > > - //HOST/b is mounted on /mnt/b > > > > On a slow connection, running 'df' and killing it while it's > > processing /mnt/b can make cifs_get_inode_info() returns -ERESTARTSYS. > > > > This triggers the following chain of events: > > => the dentry revalidation fail > > => dentry is put and released > > => superblock associated with the dentry is put > > => /mnt/b is unmounted > > > > This patch makes cifs_d_revalidate() return the error instead of 0 > > (invalid) when cifs_revalidate_dentry() fails, except for ENOENT (file > > deleted) and ESTALE (file recreated). > > > > Signed-off-by: Aurelien Aptel <aaptel@suse.com> > > Suggested-by: Shyam Prasad N <nspmangalore@gmail.com> > > CC: stable@vger.kernel.org > > > > --- > > fs/cifs/dir.c | 22 ++++++++++++++++++++-- > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c > > index 68900f1629bff..97ac363b5df16 100644 > > --- a/fs/cifs/dir.c > > +++ b/fs/cifs/dir.c > > @@ -737,6 +737,7 @@ static int > > cifs_d_revalidate(struct dentry *direntry, unsigned int flags) > > { > > struct inode *inode; > > + int rc; > > > > if (flags & LOOKUP_RCU) > > return -ECHILD; > > @@ -746,8 +747,25 @@ cifs_d_revalidate(struct dentry *direntry, unsigned int flags) > > if ((flags & LOOKUP_REVAL) && !CIFS_CACHE_READ(CIFS_I(inode))) > > CIFS_I(inode)->time = 0; /* force reval */ > > > > - if (cifs_revalidate_dentry(direntry)) > > - return 0; > > + rc = cifs_revalidate_dentry(direntry); > > + if (rc) { > > + cifs_dbg(FYI, "cifs_revalidate_dentry failed with rc=%d", rc); > > + switch (rc) { > > + case -ENOENT: > > + case -ESTALE: > > + /* > > + * Those errors mean the dentry is invalid > > + * (file was deleted or recreated) > > + */ > > + return 0; > > + default: > > + /* > > + * Otherwise some unexpected error happened > > + * report it as-is to VFS layer > > + */ > > + return rc; > > + } > > + } > > else { > > /* > > * If the inode wasn't known to be a dfs entry when > > -- > > 2.29.2 > > > > > -- > Regards, > Shyam
Tentatively merged into cifs-2.6.git for-next, since it seems to have fixed the problem we saw with test generic/070 On Fri, Feb 5, 2021 at 8:42 AM Aurélien Aptel <aaptel@suse.com> wrote: > > From: Aurelien Aptel <aaptel@suse.com> > > Assuming > - //HOST/a is mounted on /mnt > - //HOST/b is mounted on /mnt/b > > On a slow connection, running 'df' and killing it while it's > processing /mnt/b can make cifs_get_inode_info() returns -ERESTARTSYS. > > This triggers the following chain of events: > => the dentry revalidation fail > => dentry is put and released > => superblock associated with the dentry is put > => /mnt/b is unmounted > > This patch makes cifs_d_revalidate() return the error instead of 0 > (invalid) when cifs_revalidate_dentry() fails, except for ENOENT (file > deleted) and ESTALE (file recreated). > > Signed-off-by: Aurelien Aptel <aaptel@suse.com> > Suggested-by: Shyam Prasad N <nspmangalore@gmail.com> > CC: stable@vger.kernel.org > > --- > fs/cifs/dir.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c > index 68900f1629bff..97ac363b5df16 100644 > --- a/fs/cifs/dir.c > +++ b/fs/cifs/dir.c > @@ -737,6 +737,7 @@ static int > cifs_d_revalidate(struct dentry *direntry, unsigned int flags) > { > struct inode *inode; > + int rc; > > if (flags & LOOKUP_RCU) > return -ECHILD; > @@ -746,8 +747,25 @@ cifs_d_revalidate(struct dentry *direntry, unsigned int flags) > if ((flags & LOOKUP_REVAL) && !CIFS_CACHE_READ(CIFS_I(inode))) > CIFS_I(inode)->time = 0; /* force reval */ > > - if (cifs_revalidate_dentry(direntry)) > - return 0; > + rc = cifs_revalidate_dentry(direntry); > + if (rc) { > + cifs_dbg(FYI, "cifs_revalidate_dentry failed with rc=%d", rc); > + switch (rc) { > + case -ENOENT: > + case -ESTALE: > + /* > + * Those errors mean the dentry is invalid > + * (file was deleted or recreated) > + */ > + return 0; > + default: > + /* > + * Otherwise some unexpected error happened > + * report it as-is to VFS layer > + */ > + return rc; > + } > + } > else { > /* > * If the inode wasn't known to be a dfs entry when > -- > 2.29.2 > -- Thanks, Steve
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c index 68900f1629bff..97ac363b5df16 100644 --- a/fs/cifs/dir.c +++ b/fs/cifs/dir.c @@ -737,6 +737,7 @@ static int cifs_d_revalidate(struct dentry *direntry, unsigned int flags) { struct inode *inode; + int rc; if (flags & LOOKUP_RCU) return -ECHILD; @@ -746,8 +747,25 @@ cifs_d_revalidate(struct dentry *direntry, unsigned int flags) if ((flags & LOOKUP_REVAL) && !CIFS_CACHE_READ(CIFS_I(inode))) CIFS_I(inode)->time = 0; /* force reval */ - if (cifs_revalidate_dentry(direntry)) - return 0; + rc = cifs_revalidate_dentry(direntry); + if (rc) { + cifs_dbg(FYI, "cifs_revalidate_dentry failed with rc=%d", rc); + switch (rc) { + case -ENOENT: + case -ESTALE: + /* + * Those errors mean the dentry is invalid + * (file was deleted or recreated) + */ + return 0; + default: + /* + * Otherwise some unexpected error happened + * report it as-is to VFS layer + */ + return rc; + } + } else { /* * If the inode wasn't known to be a dfs entry when