Re: [PATCH] nfs: don't invalidate dentries on transient errors

[Trond Myklebust <trondmy@hammerspace.com>]

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@redhat.com>
> ---
>  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@hammerspace.com