On Fri, Feb 5, 2021 at 8:52 AM Shyam Prasad N [off-list ref] 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 [off-list ref] wrote:

quoted

From: Aurelien Aptel <redacted>

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 <redacted>
Suggested-by: Shyam Prasad N <redacted>
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

-- 
Thanks,

Steve