[PATCH] nfsd: fix file change detection in CB_GETATTR

[PATCH] nfsd: fix file change detection in CB_GETATTR

[Scott Mayhew]

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,
-- 
2.53.0

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

[Jeff Layton]

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>

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

[Chuck Lever]

On Fri, Apr 3, 2026, at 9:22 AM, 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.

Thanks for the patch! I prefer patches against nfsd-testing, though
this patch applied just fine there. This fix will likely go in via
an early 7.1-rc PR.


> 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)

The kernel-doc comment for nfsd4_deleg_getattr_conflict also
needs to be updated since you have modified the name of one
of its parameters.


>  {
>  	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);

Nit: relocate this declaration to maintain reverse-christmas
tree ordering

> +	struct kstat stat;

A minor concern here -- The caller already allocates its own
struct kstat via struct nfsd4_fattr_args. Two kstat instances
on the stack simultaneously is meaningful on 8KB stacks. Worth
verifying with scripts/checkstack.pl, especially under
CONFIG_KASAN or CONFIG_FRAME_WARN builds.


> +	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;
> +	}

When ncf_file_modified is already true (set on a prior call),
the check at line 9527 is skipped, so stat.size is never used.
Perhaps the new vfs_getattr() could be skipped in that case.

Also, after a successful delegation recall, proceeding with
CB_GETATTR comparison logic is probably unnecessary. But that
particular control flow predates your patch.


>  	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;


-- 
Chuck Lever