Re: [PATCH] nfsd: Don't reset the write verifier on a commit EAGAIN

[Chuck Lever <chuck.lever@oracle.com>]

On Mon, Sep 11, 2023 at 02:43:57PM -0400, trondmy@gmail.com wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> If fsync() is returning EAGAIN, then we can assume that the filesystem
> being exported is something like NFS with the 'softerr' mount option
> enabled, and that it is just asking us to replay the fsync() operation
> at a later date.
> If we see an ESTALE, then ditto: the file is gone, so there is no danger
> of losing the error.
> For those cases, do not reset the write verifier.

Out of interest, what's the hazard in a write verifier change in
these cases? There could be a slight performance penalty, I imagine,
but how frequently does this happen?

One more below.


> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfsd/vfs.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 98fa4fd0556d..31daf9f63572 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -337,6 +337,20 @@ nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name,
>  	return err;
>  }
>  
> +static void
> +commit_reset_write_verifier(struct nfsd_net *nn, struct svc_rqst *rqstp,
> +			    int err)
> +{
> +	switch (err) {
> +	case -EAGAIN:
> +	case -ESTALE:
> +		break;
> +	default:
> +		nfsd_reset_write_verifier(nn);
> +		trace_nfsd_writeverf_reset(nn, rqstp, err);
> +	}
> +}
> +
>  /*
>   * Commit metadata changes to stable storage.
>   */
> @@ -647,8 +661,7 @@ __be32 nfsd4_clone_file_range(struct svc_rqst *rqstp,
>  					&nfsd4_get_cstate(rqstp)->current_fh,
>  					dst_pos,
>  					count, status);
> -			nfsd_reset_write_verifier(nn);
> -			trace_nfsd_writeverf_reset(nn, rqstp, status);
> +			commit_reset_write_verifier(nn, rqstp, status);
>  			ret = nfserrno(status);
>  		}
>  	}
> @@ -1170,8 +1183,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf,
>  	host_err = vfs_iter_write(file, &iter, &pos, flags);
>  	file_end_write(file);
>  	if (host_err < 0) {
> -		nfsd_reset_write_verifier(nn);
> -		trace_nfsd_writeverf_reset(nn, rqstp, host_err);
> +		commit_reset_write_verifier(nn, rqstp, host_err);

Can generic_file_write_iter() or its brethren return STALE or AGAIN
before they get to the generic_write_sync() call ?


>  		goto out_nfserr;
>  	}
>  	*cnt = host_err;
> @@ -1183,10 +1195,8 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf,
>  
>  	if (stable && use_wgather) {
>  		host_err = wait_for_concurrent_writes(file);
> -		if (host_err < 0) {
> -			nfsd_reset_write_verifier(nn);
> -			trace_nfsd_writeverf_reset(nn, rqstp, host_err);
> -		}
> +		if (host_err < 0)
> +			commit_reset_write_verifier(nn, rqstp, host_err);
>  	}
>  
>  out_nfserr:
> @@ -1329,8 +1339,7 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf,
>  			err = nfserr_notsupp;
>  			break;
>  		default:
> -			nfsd_reset_write_verifier(nn);
> -			trace_nfsd_writeverf_reset(nn, rqstp, err2);
> +			commit_reset_write_verifier(nn, rqstp, err2);
>  			err = nfserrno(err2);
>  		}
>  	} else
> -- 
> 2.41.0
> 

-- 
Chuck Lever