On Wed, 22 May 2024, Trond Myklebust wrote:
> On Wed, 2024-05-22 at 18:19 -0400, Scott Mayhew wrote:
> > This is a slight variation on a patch previously proposed by Neil
> > Brown
> > that never got merged.
> >
> > Prior to commit 5ceb9d7fdaaf ("NFS: Refactor
> > nfs_lookup_revalidate()"),
> > any error from nfs_lookup_verify_inode() other than -ESTALE would
> > result
> > in nfs_lookup_revalidate() returning that error (-ESTALE is mapped to
> > zero).
> >
> > Since that commit, all errors result in nfs_lookup_revalidate()
> > returning zero, resulting in dentries being invalidated where they
> > previously were not (particularly in the case of -ERESTARTSYS).
> >
> > Fix it by passing the actual error code to
> > nfs_lookup_revalidate_done(),
> > and leaving the decision on whether to map the error code to zero or
> > one to nfs_lookup_revalidate_done().
> >
> > A simple reproducer is to run the following python code in a
> > subdirectory of an NFS mount (not in the root of the NFS mount):
> >
> > ---8<---
> > import os
> > import multiprocessing
> > import time
> >
> > if __name__=="__main__":
> > multiprocessing.set_start_method("spawn")
> >
> > count = 0
> > while True:
> > try:
> > os.getcwd()
> > pool = multiprocessing.Pool(10)
> > pool.close()
> > pool.terminate()
> > count += 1
> > except Exception as e:
> > print(f"Failed after {count} iterations")
> > print(e)
> > break
> > ---8<---
> >
> > Prior to commit 5ceb9d7fdaaf, the above code would run indefinitely.
> > After commit 5ceb9d7fdaaf, it fails almost immediately with -ENOENT.
> >
> > Signed-off-by: Scott Mayhew <smayhew@xxxxxxxxxx>
> > ---
> > fs/nfs/dir.c | 24 +++++++++++-------------
> > 1 file changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index ac505671efbd..d9264ed4ac52 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -1635,6 +1635,14 @@ nfs_lookup_revalidate_done(struct inode *dir,
> > struct dentry *dentry,
> > if (inode && IS_ROOT(dentry))
> > error = 1;
> > break;
> > + case -ESTALE:
> > + case -ENOENT:
> > + error = 0;
> > + break;
> > + case -ETIMEDOUT:
> > + if (NFS_SERVER(inode)->flags & NFS_MOUNT_SOFTREVAL)
> > + error = 1;
> > + break;
> > }
> > trace_nfs_lookup_revalidate_exit(dir, dentry, 0, error);
> > return error;
> > @@ -1680,18 +1688,8 @@ static int nfs_lookup_revalidate_dentry(struct
> > inode *dir,
> >
> > dir_verifier = nfs_save_change_attribute(dir);
> > ret = NFS_PROTO(dir)->lookup(dir, dentry, fhandle, fattr);
> > - if (ret < 0) {
> > - switch (ret) {
> > - case -ESTALE:
> > - case -ENOENT:
> > - ret = 0;
> > - break;
> > - case -ETIMEDOUT:
> > - if (NFS_SERVER(inode)->flags &
> > NFS_MOUNT_SOFTREVAL)
> > - ret = 1;
> > - }
> > + if (ret < 0)
> > goto out;
> > - }
> >
> > /* Request help from readdirplus */
> > nfs_lookup_advise_force_readdirplus(dir, flags);
> > @@ -1735,7 +1733,7 @@ nfs_do_lookup_revalidate(struct inode *dir,
> > struct dentry *dentry,
> > unsigned int flags)
> > {
> > struct inode *inode;
> > - int error;
> > + int error = 0;
> >
> > nfs_inc_stats(dir, NFSIOS_DENTRYREVALIDATE);
> > inode = d_inode(dentry);
> > @@ -1780,7 +1778,7 @@ nfs_do_lookup_revalidate(struct inode *dir,
> > struct dentry *dentry,
> > out_bad:
> > if (flags & LOOKUP_RCU)
> > return -ECHILD;
> > - return nfs_lookup_revalidate_done(dir, dentry, inode, 0);
> > + return nfs_lookup_revalidate_done(dir, dentry, inode,
> > error);
>
> Won't this now cause us to skip the special handling of the root
> directory in nfs_lookup_revalidate_done() if the call to
> nfs_lookup_verify_inode() fails with an error?
Yes, it will. I'll send a v2 in a bit.
-Scott
>
> > }
> >
> > static int
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@xxxxxxxxxxxxxxx
>
>
