Re: [PATCH v4] cifs: report error instead of invalid when revalidating a dentry fails

[Steve French <smfrench@gmail.com>]

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



-- 
Thanks,

Steve