[PATCH] nfsd: don't set the ctime on delegated atime updates

[PATCH] nfsd: don't set the ctime on delegated atime updates

[Jeff Layton]

Clients will typically precede a DELEGRETURN for a delegation with
delegated timestamp with a SETATTR to set the timestamps on the server
to match what the client has.

knfsd implements this by using the nfsd_setattr() infrastructure, which
will set ATTR_CTIME on any update that goes to notify_change(). This is
problematic as it means that the client will get a spurious ctime
updates when updating the atime.

Fix this by pushing the handling of ATTR_CTIME down into the decoder
functions so that they are set earlier and more deliberately, and stop
nfsd_setattr() from implicitly adding it to every notify_change() call.

Fixes: 7e13f4f8d27d ("nfsd: handle delegated timestamps in SETATTR")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
I finally was able to spend some time on the gitr failures with
delegated timestamps and found this. With this patch I was able to run
the gitr suite in the "stress" configuration under kdevops and it
passed.
---
 fs/nfsd/nfs3xdr.c   |  3 +++
 fs/nfsd/nfs4state.c |  2 +-
 fs/nfsd/nfs4xdr.c   | 18 +++++++++---------
 fs/nfsd/nfsxdr.c    |  3 +++
 fs/nfsd/vfs.c       |  1 -
 5 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index ef4971d71ac4bfee5171537eda80dec6e367060d..3058c73fe2d204131c33e31e76f7ca15c2545d36 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -289,6 +289,9 @@ svcxdr_decode_sattr3(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 		return false;
 	}
 
+	if (iap->ia_valid)
+		iap->ia_valid |= ATTR_CTIME;
+
 	return true;
 }
 
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d5694987f86fadab985e55cce6261ad680e83b69..3aa430c8d941570840fc482efc750daa5df3ae84 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5690,7 +5690,7 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh,
 		struct nfsd4_open *open)
 {
 	struct iattr iattr = {
-		.ia_valid = ATTR_SIZE,
+		.ia_valid = ATTR_SIZE | ATTR_CTIME,
 		.ia_size = 0,
 	};
 	struct nfsd_attrs attrs = {
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 3afcdbed6e1465929ec7da67593243a8bf97b3f4..568b30619c79a857429113015b2ff7081557681f 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -409,7 +409,7 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
 		if (xdr_stream_decode_u64(argp->xdr, &size) < 0)
 			return nfserr_bad_xdr;
 		iattr->ia_size = size;
-		iattr->ia_valid |= ATTR_SIZE;
+		iattr->ia_valid |= ATTR_SIZE | ATTR_CTIME;
 	}
 	if (bmval[0] & FATTR4_WORD0_ACL) {
 		status = nfsd4_decode_acl(argp, acl);
@@ -424,7 +424,7 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
 			return nfserr_bad_xdr;
 		iattr->ia_mode = mode;
 		iattr->ia_mode &= (S_IFMT | S_IALLUGO);
-		iattr->ia_valid |= ATTR_MODE;
+		iattr->ia_valid |= ATTR_MODE | ATTR_CTIME;
 	}
 	if (bmval[1] & FATTR4_WORD1_OWNER) {
 		u32 length;
@@ -438,7 +438,7 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
 					      &iattr->ia_uid);
 		if (status)
 			return status;
-		iattr->ia_valid |= ATTR_UID;
+		iattr->ia_valid |= ATTR_UID | ATTR_CTIME;
 	}
 	if (bmval[1] & FATTR4_WORD1_OWNER_GROUP) {
 		u32 length;
@@ -452,7 +452,7 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
 					      &iattr->ia_gid);
 		if (status)
 			return status;
-		iattr->ia_valid |= ATTR_GID;
+		iattr->ia_valid |= ATTR_GID | ATTR_CTIME;
 	}
 	if (bmval[1] & FATTR4_WORD1_TIME_ACCESS_SET) {
 		u32 set_it;
@@ -464,10 +464,10 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
 			status = nfsd4_decode_nfstime4(argp, &iattr->ia_atime);
 			if (status)
 				return status;
-			iattr->ia_valid |= (ATTR_ATIME | ATTR_ATIME_SET);
+			iattr->ia_valid |= ATTR_ATIME | ATTR_ATIME_SET | ATTR_CTIME;
 			break;
 		case NFS4_SET_TO_SERVER_TIME:
-			iattr->ia_valid |= ATTR_ATIME;
+			iattr->ia_valid |= ATTR_ATIME | ATTR_CTIME;
 			break;
 		default:
 			return nfserr_bad_xdr;
@@ -492,10 +492,10 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
 			status = nfsd4_decode_nfstime4(argp, &iattr->ia_mtime);
 			if (status)
 				return status;
-			iattr->ia_valid |= (ATTR_MTIME | ATTR_MTIME_SET);
+			iattr->ia_valid |= ATTR_MTIME | ATTR_MTIME_SET | ATTR_CTIME;
 			break;
 		case NFS4_SET_TO_SERVER_TIME:
-			iattr->ia_valid |= ATTR_MTIME;
+			iattr->ia_valid |= ATTR_MTIME | ATTR_CTIME;
 			break;
 		default:
 			return nfserr_bad_xdr;
@@ -519,7 +519,7 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
 		if (xdr_stream_decode_u32(argp->xdr, &mask) < 0)
 			return nfserr_bad_xdr;
 		*umask = mask & S_IRWXUGO;
-		iattr->ia_valid |= ATTR_MODE;
+		iattr->ia_valid |= ATTR_MODE | ATTR_CTIME;
 	}
 	if (bmval[2] & FATTR4_WORD2_TIME_DELEG_ACCESS) {
 		fattr4_time_deleg_access access;
diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
index fc262ceafca972df353a389eea2d7e99fd6bc1d2..8372a374623a5d98fff7c0227a73a9b562da407d 100644
--- a/fs/nfsd/nfsxdr.c
+++ b/fs/nfsd/nfsxdr.c
@@ -196,6 +196,9 @@ svcxdr_decode_sattr(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 			iap->ia_valid &= ~(ATTR_ATIME_SET|ATTR_MTIME_SET);
 	}
 
+	if (iap->ia_valid)
+		iap->ia_valid |= ATTR_CTIME;
+
 	return true;
 }
 
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index cd689df2ca5d7396cffb5ed9dc14f774a8f3881c..383a724e9aa20a4c6a71a563281fc8a618dd36d3 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -470,7 +470,6 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
 	if (!iap->ia_valid)
 		return 0;
 
-	iap->ia_valid |= ATTR_CTIME;
 	return notify_change(&nop_mnt_idmap, dentry, iap, NULL);
 }
 

---
base-commit: 347e9f5043c89695b01e66b3ed111755afcf1911
change-id: 20250715-tsfix-780246904a01

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] nfsd: don't set the ctime on delegated atime updates

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

On Tue, 15 Jul 2025 07:34:10 -0400, Jeff Layton wrote:
> Clients will typically precede a DELEGRETURN for a delegation with
> delegated timestamp with a SETATTR to set the timestamps on the server
> to match what the client has.
> 
> knfsd implements this by using the nfsd_setattr() infrastructure, which
> will set ATTR_CTIME on any update that goes to notify_change(). This is
> problematic as it means that the client will get a spurious ctime
> updates when updating the atime.
> 
> [...]

Applied to nfsd-testing, thanks!

Because this patch touches generic attribute handling, I'm targeting
it for v6.18.

[1/1] nfsd: don't set the ctime on delegated atime updates
      commit: aaf2b07a551950a6e4deab07f6cf73c4a6592a35

--
Chuck Lever

Re: [PATCH] nfsd: don't set the ctime on delegated atime updates

[Chuck Lever]

On 7/15/25 7:34 AM, Jeff Layton wrote:
> Clients will typically precede a DELEGRETURN for a delegation with
> delegated timestamp with a SETATTR to set the timestamps on the server
> to match what the client has.
> 
> knfsd implements this by using the nfsd_setattr() infrastructure, which
> will set ATTR_CTIME on any update that goes to notify_change(). This is
> problematic as it means that the client will get a spurious ctime
> updates when updating the atime.
> 
> Fix this by pushing the handling of ATTR_CTIME down into the decoder
> functions so that they are set earlier and more deliberately, and stop
> nfsd_setattr() from implicitly adding it to every notify_change() call.
> 
> Fixes: 7e13f4f8d27d ("nfsd: handle delegated timestamps in SETATTR")
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
I was concerned about the description making a claim without reference
that modifying atime does not modify ctime. That claim has been somewhat
contentious in the past.

Search engines suggest that IEEE 1003.1 does indeed specify that an
atime modification does not require a ctime change. POSIX does not make
these standards free, however. Maybe a Linux-specific document has
something on point.

Even so, I'm comfortable taking this for now and putting it through the
normal set of regression testing.

Thank you for pursuing this one, Jeff!


-- 
Chuck Lever

Re: [PATCH] nfsd: don't set the ctime on delegated atime updates

[Jeff Layton]

On Tue, 2025-07-15 at 12:46 -0400, Chuck Lever wrote:
> On 7/15/25 7:34 AM, Jeff Layton wrote:
> > Clients will typically precede a DELEGRETURN for a delegation with
> > delegated timestamp with a SETATTR to set the timestamps on the server
> > to match what the client has.
> > 
> > knfsd implements this by using the nfsd_setattr() infrastructure, which
> > will set ATTR_CTIME on any update that goes to notify_change(). This is
> > problematic as it means that the client will get a spurious ctime
> > updates when updating the atime.
> > 
> > Fix this by pushing the handling of ATTR_CTIME down into the decoder
> > functions so that they are set earlier and more deliberately, and stop
> > nfsd_setattr() from implicitly adding it to every notify_change() call.
> > 
> > Fixes: 7e13f4f8d27d ("nfsd: handle delegated timestamps in SETATTR")
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> I was concerned about the description making a claim without reference
> that modifying atime does not modify ctime. That claim has been somewhat
> contentious in the past.
> 
> Search engines suggest that IEEE 1003.1 does indeed specify that an
> atime modification does not require a ctime change. POSIX does not make
> these standards free, however. Maybe a Linux-specific document has
> something on point.
> 

Ok, good. We probably ought to spell this out in some manpage...

> Even so, I'm comfortable taking this for now and putting it through the
> normal set of regression testing.
> 
> Thank you for pursuing this one, Jeff!
> 

Thanks. I think the principle is correct. Unfortunately, this patch
misses a place where we need to set ATTR_CTIME. You may want to drop it
for now.

I'm working on a different approach that makes it skip setting
ATTR_CTIME if ATTR_DELEG is set. ATTR_DELEG gets set on any delegated
timestamp update, so that should make it do the right thing (and the
patch will be less invasive).

Thanks for the review so far!
-- 
Jeff Layton <jlayton@kernel.org>