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?
> }
>
> static int
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
