Re: [PATCH] nfsd: fix file change detection in CB_GETATTR

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2026-04-03 at 09:22 -0400, Scott Mayhew wrote:
> RFC 8881, section 10.4.3 doesn't say anything about caching the file
> size in the delegation record, nor does it say anything about comparing
> a cached file size with the size reported by the client in the
> CB_GETATTR reply for the purpose of determining if the client holds
> modified data for the file.
> 
> What section 10.4.3 of RFC 8881 does say is that the server should
> compare the *current* file size with size reported by the client holding
> the delegation in the CB_GETATTR reply, and if they differ to treat it
> as a modification regardless of the change attribute retrieved via the
> CB_GETATTR.
> 
> Doing otherwise would cause the server to believe the client holding the
> delegation has a modified version of the file, even if the client
> flushed the modifications to the server prior to the CB_GETATTR.  This
> would have the added side effect of subsequent CB_GETATTRs causing
> updates to the mtime, ctime, and change attribute even if the client
> holding the delegation makes no further updates to the file.
> 
> Modify nfsd4_deleg_getattr_conflict() to obtain the current file size
> via vfs_getattr().  Retain the ncf_cur_fsize field, since it's a
> convenient way to return the file size back to nfsd4_encode_fattr4(),
> but don't use it for the purpose of detecting file changes.
> 
> Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
> Note this patch is against Chuck's nfsd-next branch.
> 
> Also, I have a pynfs test that illustrates the "bad" behavior.  See
> "pynfs: add delegation test for CB_GETATTR after sync WRITE", which will
> be sent shortly.
> 
>  fs/nfsd/nfs4state.c | 17 +++++++++++------
>  fs/nfsd/nfs4xdr.c   |  2 +-
>  fs/nfsd/state.h     |  2 +-
>  3 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b4d0e82b2690..2c82438918f6 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -9372,7 +9372,7 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
>   * caller must put the reference.
>   */
>  __be32
> -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
> +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct path *path,
>  			     struct nfs4_delegation **pdp)
>  {
>  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> @@ -9381,7 +9381,9 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
>  	struct nfs4_delegation *dp = NULL;
>  	struct file_lease *fl;
>  	struct nfs4_cb_fattr *ncf;
> -	struct inode *inode = d_inode(dentry);
> +	struct inode *inode = d_inode(path->dentry);
> +	struct kstat stat;
> +	int err;
>  	__be32 status;
>  
>  	ctx = locks_inode_context(inode);
> @@ -9430,19 +9432,22 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
>  		    !nfsd_wait_for_delegreturn(rqstp, inode))
>  			goto out_status;
>  	}
> +	err = vfs_getattr(path, &stat, STATX_SIZE, AT_STATX_SYNC_AS_STAT);
> +	if (err) {
> +		status = nfserrno(err);
> +		goto out_status;
> +	}
>  	if (!ncf->ncf_file_modified &&
>  	    (ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
> -	     ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
> +	     stat.size != ncf->ncf_cb_fsize))
>  		ncf->ncf_file_modified = true;
>  	if (ncf->ncf_file_modified) {
> -		int err;
> -
>  		/*
>  		 * Per section 10.4.3 of RFC 8881, the server would
>  		 * not update the file's metadata with the client's
>  		 * modified size
>  		 */
> -		err = cb_getattr_update_times(dentry, dp);
> +		err = cb_getattr_update_times(path->dentry, dp);
>  		if (err) {
>  			status = nfserrno(err);
>  			goto out_status;
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 2a0946c630e1..b380c2545f6a 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3914,7 +3914,7 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  	    (attrmask[1] & (FATTR4_WORD1_TIME_ACCESS |
>  			    FATTR4_WORD1_TIME_MODIFY |
>  			    FATTR4_WORD1_TIME_METADATA))) {
> -		status = nfsd4_deleg_getattr_conflict(rqstp, dentry, &dp);
> +		status = nfsd4_deleg_getattr_conflict(rqstp, &path, &dp);
>  		if (status)
>  			goto out;
>  	}
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 9b05462da4cc..edfb3402dfd2 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -889,7 +889,7 @@ static inline bool try_to_expire_client(struct nfs4_client *clp)
>  }
>  
>  extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp,
> -		struct dentry *dentry, struct nfs4_delegation **pdp);
> +		struct path *path, struct nfs4_delegation **pdp);
>  
>  struct nfsd4_get_dir_delegation;
>  struct nfs4_delegation *nfsd_get_dir_deleg(struct nfsd4_compound_state *cstate,

Reviewed-by: Jeff Layton <jlayton@kernel.org>