[PATCH v2 0/7] nfsd/vfs: fix handling of delegated timestamp updates

[PATCH v2 0/7] nfsd/vfs: fix handling of delegated timestamp updates

[Jeff Layton]

After my last posting, Trond pointed out that my interpretation of RFC
9754 was wrong. The "original time" as mentioned in the spec is the time
of the grant of the delegation, and not the current timestamp in the
inode before the update.

Given that, there is no longer a need to do any sort of complicated
handling of delegated timestamps at the VFS layer, so this set removes
inode_set_ctime_deleg(). We do however need a way to set a ctime that
isn't current_time() via notify_change(). This patchset adds an
ATTR_CTIME_SET flag, which mirrors ATTR_ATIME_SET and ATTR_MTIME_SET
semantics. The rest of the patchset reworks nfsd to properly vet
timestamp updates on its own and just call down into the OS to do a
normal notify_change().

With this patchset in place I haven't seen any git regression suite
failures yet (4 passes so far under kdevops in the "stress"
configuration).

I should point out that there is at least one potential problem with
this implementation:

The kernel does not block getattr operations when there is a delegation
outstanding. When the client issues read and write ops to the server,
while holding the delegation, the timestamps on the file get updated to
the server's time as usual. Later, the client will send a SETATTR (or a
CB_GETTTR reply) that may have times that are earlier than the
timestamps currently on the inode.

This means that userland applications running on the NFS server can
observe timestamps going backward. This applies even for the ctime. NFS
clients should be fine, as the server will do a CB_GETATTR to satisfy
them. Server-side applications can't do much else with the inode without
recalling the delegation, so my thinking is that the effect should be
"mostly harmless".

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v2:
- add ATTR_CTIME_SET and remove inode_set_ctime_deleg()
- track original timestamps in struct nfs4_delegation
- fix delegated timestamp updates to respect saved timestamps
- Link to v1: https://lore.kernel.org/r/20250722-nfsd-testing-v1-0-31321c7fc97f@kernel.org

---
Jeff Layton (7):
      vfs: add ATTR_CTIME_SET flag
      nfsd: ignore ATTR_DELEG when checking ia_valid before notify_change()
      nfsd: use ATTR_CTIME_SET for delegated ctime updates
      nfsd: track original timestamps in nfs4_delegation
      nfsd: fix SETATTR updates for delegated timestamps
      nfsd: fix timestamp updates in CB_GETATTR
      vfs: remove inode_set_ctime_deleg()

 fs/attr.c           | 34 ++++++++++---------------
 fs/inode.c          | 73 -----------------------------------------------------
 fs/nfsd/nfs4proc.c  | 31 ++++++++++++++++++++++-
 fs/nfsd/nfs4state.c | 44 +++++++++++++++++---------------
 fs/nfsd/nfs4xdr.c   |  5 ++--
 fs/nfsd/state.h     |  8 ++++++
 fs/nfsd/vfs.c       |  2 +-
 include/linux/fs.h  |  3 +--
 8 files changed, 79 insertions(+), 121 deletions(-)
---
base-commit: b05f077b59098b4760e3f675b00a4e6a1ad4b0ad
change-id: 20250722-nfsd-testing-5e861a3cf3a0

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

[PATCH v2 1/7] vfs: add ATTR_CTIME_SET flag

[Jeff Layton]

When ATTR_ATIME_SET and ATTR_MTIME_SET are set in the ia_valid mask, the
notify_change() logic takes that to mean that the request should set
those values explicitly, and not override them with "now".

With the advent of delegated timestamps, similar functionality is needed
for the ctime. Add a ATTR_CTIME_SET flag, and use that to indicate that
the ctime should be accepted as-is. Also, clean up the if statements to
eliminate the extra negatives.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/attr.c          | 15 +++++++++------
 include/linux/fs.h |  1 +
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index 9caf63d20d03e86c535e9c8c91d49c2a34d34b7a..f0dabd2985989d283a931536a5fc53eda366b373 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -463,15 +463,18 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
 
 	now = current_time(inode);
 
-	attr->ia_ctime = now;
-	if (!(ia_valid & ATTR_ATIME_SET))
-		attr->ia_atime = now;
-	else
+	if (ia_valid & ATTR_ATIME_SET)
 		attr->ia_atime = timestamp_truncate(attr->ia_atime, inode);
-	if (!(ia_valid & ATTR_MTIME_SET))
-		attr->ia_mtime = now;
 	else
+		attr->ia_atime = now;
+	if (ia_valid & ATTR_CTIME_SET)
+		attr->ia_ctime = timestamp_truncate(attr->ia_ctime, inode);
+	else
+		attr->ia_ctime = now;
+	if (ia_valid & ATTR_MTIME_SET)
 		attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode);
+	else
+		attr->ia_mtime = now;
 
 	if (ia_valid & ATTR_KILL_PRIV) {
 		error = security_inode_need_killpriv(dentry);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 040c0036320fdf87a2379d494ab408a7991875bd..f18f45e88545c39716b917b1378fb7248367b41d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -237,6 +237,7 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 #define ATTR_ATIME_SET	(1 << 7)
 #define ATTR_MTIME_SET	(1 << 8)
 #define ATTR_FORCE	(1 << 9) /* Not a change, but a change it */
+#define ATTR_CTIME_SET	(1 << 10)
 #define ATTR_KILL_SUID	(1 << 11)
 #define ATTR_KILL_SGID	(1 << 12)
 #define ATTR_FILE	(1 << 13)

-- 
2.50.1

[PATCH v2 2/7] nfsd: ignore ATTR_DELEG when checking ia_valid before notify_change()

[Jeff Layton]

If the only flag left is ATTR_DELEG, then there are no changes to be
made.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/vfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index eaf04751d07fe9be4d1dd08477bc5a38ac99be3a..68d42fc5504d8750174b9a2aef10886ed92f528b 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -467,7 +467,7 @@ static int __nfsd_setattr(struct dentry *dentry, struct iattr *iap)
 			return 0;
 	}
 
-	if (!iap->ia_valid)
+	if ((iap->ia_valid & ~ATTR_DELEG) == 0)
 		return 0;
 
 	/*

-- 
2.50.1

[PATCH v2 3/7] nfsd: use ATTR_CTIME_SET for delegated ctime updates

[Jeff Layton]

Ensure that notify_change() doesn't clobber a delegated ctime update
with current_time() by setting ATTR_CTIME_SET for those updates.

Also, set the tv_nsec field the nfsd4_decode_fattr4 to the correct
value.

Don't bother setting the timestamps in cb_getattr_update_times() in the
non-delegated case. notify_change() will do that itself.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 6 +++---
 fs/nfsd/nfs4xdr.c   | 5 +++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 88c347957da5b8f352be63f84f207d2225f81cb9..77eea2ad93cc07939f045fc4b983b1ac00d068b8 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -9167,7 +9167,6 @@ static bool set_cb_time(struct timespec64 *cb, const struct timespec64 *orig,
 static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation *dp)
 {
 	struct inode *inode = d_inode(dentry);
-	struct timespec64 now = current_time(inode);
 	struct nfs4_cb_fattr *ncf = &dp->dl_cb_fattr;
 	struct iattr attrs = { };
 	int ret;
@@ -9175,6 +9174,7 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
 	if (deleg_attrs_deleg(dp->dl_type)) {
 		struct timespec64 atime = inode_get_atime(inode);
 		struct timespec64 mtime = inode_get_mtime(inode);
+		struct timespec64 now = current_time(inode);
 
 		attrs.ia_atime = ncf->ncf_cb_atime;
 		attrs.ia_mtime = ncf->ncf_cb_mtime;
@@ -9183,12 +9183,12 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
 			attrs.ia_valid |= ATTR_ATIME | ATTR_ATIME_SET;
 
 		if (set_cb_time(&attrs.ia_mtime, &mtime, &now)) {
-			attrs.ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET;
+			attrs.ia_valid |= ATTR_CTIME | ATTR_CTIME_SET |
+					  ATTR_MTIME | ATTR_MTIME_SET;
 			attrs.ia_ctime = attrs.ia_mtime;
 		}
 	} else {
 		attrs.ia_valid |= ATTR_MTIME | ATTR_CTIME;
-		attrs.ia_mtime = attrs.ia_ctime = now;
 	}
 
 	if (!attrs.ia_valid)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 8b68f74a8cf08c6aa1305a2a3093467656085e4a..c0a3c6a7c8bb70d62940115c3101e9f897401456 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -538,8 +538,9 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
 		iattr->ia_mtime.tv_sec = modify.seconds;
 		iattr->ia_mtime.tv_nsec = modify.nseconds;
 		iattr->ia_ctime.tv_sec = modify.seconds;
-		iattr->ia_ctime.tv_nsec = modify.seconds;
-		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
+		iattr->ia_ctime.tv_nsec = modify.nseconds;
+		iattr->ia_valid |= ATTR_CTIME | ATTR_CTIME_SET |
+				   ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
 	}
 
 	/* request sanity: did attrlist4 contain the expected number of words? */

-- 
2.50.1

[PATCH v2 4/7] nfsd: track original timestamps in nfs4_delegation

[Jeff Layton]

As Trond points out [1], the "original time" mentioned in RFC 9754
refers to the timestamps on the files at the time that the delegation
was granted, and not the current timestamp of the file on the server.

Store the current timestamps for the file in the nfs4_delegation when
granting one. Add STATX_ATIME and STATX_MTIME to the request mask in
nfs4_delegation_stat(). When granting OPEN_DELEGATE_READ_ATTRS_DELEG, do
a nfs4_delegation_stat() and save the correct atime. If the stat() fails
for any reason, fall back to granting a normal read deleg.

[1]: https://lore.kernel.org/linux-nfs/47a4e40310e797f21b5137e847b06bb203d99e66.camel@kernel.org/

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 11 ++++++++---
 fs/nfsd/state.h     |  5 +++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 77eea2ad93cc07939f045fc4b983b1ac00d068b8..8737b721daf3433bab46065e751175a4dcdd1c89 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6157,7 +6157,8 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
 	path.dentry = file_dentry(nf->nf_file);
 
 	rc = vfs_getattr(&path, stat,
-			 (STATX_MODE | STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE),
+			 STATX_MODE | STATX_SIZE | STATX_ATIME |
+			 STATX_MTIME | STATX_CTIME | STATX_CHANGE_COOKIE,
 			 AT_STATX_SYNC_AS_STAT);
 
 	nfsd_file_put(nf);
@@ -6274,10 +6275,14 @@ nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
 						    OPEN_DELEGATE_WRITE;
 		dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
 		dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat);
+		dp->dl_atime = stat.atime;
+		dp->dl_ctime = stat.ctime;
+		dp->dl_mtime = stat.mtime;
 		trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
 	} else {
-		open->op_delegate_type = deleg_ts ? OPEN_DELEGATE_READ_ATTRS_DELEG :
-						    OPEN_DELEGATE_READ;
+		open->op_delegate_type = deleg_ts && nfs4_delegation_stat(dp, currentfh, &stat) ?
+					 OPEN_DELEGATE_READ_ATTRS_DELEG : OPEN_DELEGATE_READ;
+		dp->dl_atime = stat.atime;
 		trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
 	}
 	nfs4_put_stid(&dp->dl_stid);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 8adc2550129e67a4e6646395fa2811e1c2acb98e..ce7c0d129ba338e1269ed163266e1ee192cd02c5 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -224,6 +224,11 @@ struct nfs4_delegation {
 
 	/* for CB_GETATTR */
 	struct nfs4_cb_fattr    dl_cb_fattr;
+
+	/* For delegated timestamps */
+	struct timespec64	dl_atime;
+	struct timespec64	dl_mtime;
+	struct timespec64	dl_ctime;
 };
 
 static inline bool deleg_is_read(u32 dl_type)

-- 
2.50.1

[PATCH v2 5/7] nfsd: fix SETATTR updates for delegated timestamps

[Jeff Layton]

SETATTRs containing delegated timestamp updates are currently not being
vetted properly. Since we no longer need to compare the timestamps vs.
the current timestamps, move the vetting of delegated timestamps wholly
into nfsd.

Rename the set_cb_time() helper to nfsd4_vet_deleg_time(), and make it
non-static. Add a new vet_deleg_attrs() helper that is called from
nfsd4_setattr that uses nfsd4_vet_deleg_time() to properly validate the
all the timestamps. If the validation indicates that the update should
be skipped, unset the appropriate flags in ia_valid.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4proc.c  | 31 ++++++++++++++++++++++++++++++-
 fs/nfsd/nfs4state.c | 24 +++++++++++-------------
 fs/nfsd/state.h     |  3 +++
 3 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 7043fc475458d3b602010b47f489a3caba85e3ca..aacd912a5fbe29ba5ccac206d13243308f36b7fa 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1133,6 +1133,33 @@ nfsd4_secinfo_no_name_release(union nfsd4_op_u *u)
 		exp_put(u->secinfo_no_name.sin_exp);
 }
 
+/*
+ * Validate that the requested timestamps are within the acceptable range. If
+ * timestamp appears to be in the future, then it will be clamped to
+ * current_time().
+ */
+static void
+vet_deleg_attrs(struct nfsd4_setattr *setattr, struct nfs4_delegation *dp)
+{
+	struct timespec64 now = current_time(dp->dl_stid.sc_file->fi_inode);
+	struct iattr *iattr = &setattr->sa_iattr;
+
+	if ((setattr->sa_bmval[2] & FATTR4_WORD2_TIME_DELEG_ACCESS) &&
+	    !nfsd4_vet_deleg_time(&iattr->ia_atime, &dp->dl_atime, &now))
+		iattr->ia_valid &= ~(ATTR_ATIME | ATTR_ATIME_SET);
+
+	if (setattr->sa_bmval[2] & FATTR4_WORD2_TIME_DELEG_MODIFY) {
+		if (nfsd4_vet_deleg_time(&iattr->ia_mtime, &dp->dl_mtime, &now)) {
+			iattr->ia_ctime = iattr->ia_mtime;
+			if (!nfsd4_vet_deleg_time(&iattr->ia_ctime, &dp->dl_ctime, &now))
+				iattr->ia_valid &= ~(ATTR_CTIME | ATTR_CTIME_SET);
+		} else {
+			iattr->ia_valid &= ~(ATTR_CTIME | ATTR_CTIME_SET |
+					     ATTR_MTIME | ATTR_MTIME_SET);
+		}
+	}
+}
+
 static __be32
 nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	      union nfsd4_op_u *u)
@@ -1170,8 +1197,10 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			struct nfs4_delegation *dp = delegstateid(st);
 
 			/* Only for *_ATTRS_DELEG flavors */
-			if (deleg_attrs_deleg(dp->dl_type))
+			if (deleg_attrs_deleg(dp->dl_type)) {
+				vet_deleg_attrs(setattr, dp);
 				status = nfs_ok;
+			}
 		}
 	}
 	if (st)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8737b721daf3433bab46065e751175a4dcdd1c89..f2fd0cbe256b9519eaa5cb0cc18872e08020edd3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -9135,25 +9135,25 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
 }
 
 /**
- * set_cb_time - vet and set the timespec for a cb_getattr update
- * @cb: timestamp from the CB_GETATTR response
+ * nfsd4_vet_deleg_time - vet and set the timespec for a delegated timestamp update
+ * @req: timestamp from the client
  * @orig: original timestamp in the inode
  * @now: current time
  *
- * Given a timestamp in a CB_GETATTR response, check it against the
+ * Given a timestamp from the client response, check it against the
  * current timestamp in the inode and the current time. Returns true
  * if the inode's timestamp needs to be updated, and false otherwise.
- * @cb may also be changed if the timestamp needs to be clamped.
+ * @req may also be changed if the timestamp needs to be clamped.
  */
-static bool set_cb_time(struct timespec64 *cb, const struct timespec64 *orig,
-			const struct timespec64 *now)
+bool nfsd4_vet_deleg_time(struct timespec64 *req, const struct timespec64 *orig,
+			  const struct timespec64 *now)
 {
 
 	/*
 	 * "When the time presented is before the original time, then the
 	 *  update is ignored." Also no need to update if there is no change.
 	 */
-	if (timespec64_compare(cb, orig) <= 0)
+	if (timespec64_compare(req, orig) <= 0)
 		return false;
 
 	/*
@@ -9161,10 +9161,8 @@ static bool set_cb_time(struct timespec64 *cb, const struct timespec64 *orig,
 	 *  clamp the new time to the current time, or it may
 	 *  return NFS4ERR_DELAY to the client, allowing it to retry."
 	 */
-	if (timespec64_compare(cb, now) > 0) {
-		/* clamp it */
-		*cb = *now;
-	}
+	if (timespec64_compare(req, now) > 0)
+		*req = *now;
 
 	return true;
 }
@@ -9184,10 +9182,10 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
 		attrs.ia_atime = ncf->ncf_cb_atime;
 		attrs.ia_mtime = ncf->ncf_cb_mtime;
 
-		if (set_cb_time(&attrs.ia_atime, &atime, &now))
+		if (nfsd4_vet_deleg_time(&attrs.ia_atime, &atime, &now))
 			attrs.ia_valid |= ATTR_ATIME | ATTR_ATIME_SET;
 
-		if (set_cb_time(&attrs.ia_mtime, &mtime, &now)) {
+		if (nfsd4_vet_deleg_time(&attrs.ia_mtime, &mtime, &now)) {
 			attrs.ia_valid |= ATTR_CTIME | ATTR_CTIME_SET |
 					  ATTR_MTIME | ATTR_MTIME_SET;
 			attrs.ia_ctime = attrs.ia_mtime;
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index ce7c0d129ba338e1269ed163266e1ee192cd02c5..bf9436cdb93c5dd5502ecf83433ea311e3678711 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -247,6 +247,9 @@ static inline bool deleg_attrs_deleg(u32 dl_type)
 	       dl_type == OPEN_DELEGATE_WRITE_ATTRS_DELEG;
 }
 
+bool nfsd4_vet_deleg_time(struct timespec64 *cb, const struct timespec64 *orig,
+			  const struct timespec64 *now);
+
 #define cb_to_delegation(cb) \
 	container_of(cb, struct nfs4_delegation, dl_recall)
 

-- 
2.50.1

[PATCH v2 6/7] nfsd: fix timestamp updates in CB_GETATTR

[Jeff Layton]

When updating the local timestamps from CB_GETATTR, the updated values
are not being properly vetted.

Compare the update times vs. the saved times in the delegation rather
than the current times in the inode. Also, ensure that the ctime is
properly vetted vs. its original value.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f2fd0cbe256b9519eaa5cb0cc18872e08020edd3..205ee8cc6fa2b9f74d08f7938b323d03bdf8286c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -9175,20 +9175,19 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
 	int ret;
 
 	if (deleg_attrs_deleg(dp->dl_type)) {
-		struct timespec64 atime = inode_get_atime(inode);
-		struct timespec64 mtime = inode_get_mtime(inode);
 		struct timespec64 now = current_time(inode);
 
 		attrs.ia_atime = ncf->ncf_cb_atime;
 		attrs.ia_mtime = ncf->ncf_cb_mtime;
 
-		if (nfsd4_vet_deleg_time(&attrs.ia_atime, &atime, &now))
+		if (nfsd4_vet_deleg_time(&attrs.ia_atime, &dp->dl_atime, &now))
 			attrs.ia_valid |= ATTR_ATIME | ATTR_ATIME_SET;
 
-		if (nfsd4_vet_deleg_time(&attrs.ia_mtime, &mtime, &now)) {
-			attrs.ia_valid |= ATTR_CTIME | ATTR_CTIME_SET |
-					  ATTR_MTIME | ATTR_MTIME_SET;
+		if (nfsd4_vet_deleg_time(&attrs.ia_mtime, &dp->dl_mtime, &now)) {
+			attrs.ia_valid |= ATTR_MTIME | ATTR_MTIME_SET;
 			attrs.ia_ctime = attrs.ia_mtime;
+			if (nfsd4_vet_deleg_time(&attrs.ia_ctime, &dp->dl_ctime, &now))
+				attrs.ia_valid |= ATTR_CTIME | ATTR_CTIME_SET;
 		}
 	} else {
 		attrs.ia_valid |= ATTR_MTIME | ATTR_CTIME;

-- 
2.50.1

[PATCH v2 7/7] vfs: remove inode_set_ctime_deleg()

[Jeff Layton]

Now that nfsd is vetting the timestamps internally, there is no need for
this function. If ATTR_DELEG is set, then skip the multigrain update and
set what was requested.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/attr.c          | 19 +++-----------
 fs/inode.c         | 73 ------------------------------------------------------
 include/linux/fs.h |  2 --
 3 files changed, 4 insertions(+), 90 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index f0dabd2985989d283a931536a5fc53eda366b373..e75f06b760015640bafd596457cd14c746c7e272 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -287,14 +287,7 @@ static void setattr_copy_mgtime(struct inode *inode, const struct iattr *attr)
 	struct timespec64 now;
 
 	if (ia_valid & ATTR_CTIME) {
-		/*
-		 * In the case of an update for a write delegation, we must respect
-		 * the value in ia_ctime and not use the current time.
-		 */
-		if (ia_valid & ATTR_DELEG)
-			now = inode_set_ctime_deleg(inode, attr->ia_ctime);
-		else
-			now = inode_set_ctime_current(inode);
+		now = inode_set_ctime_current(inode);
 	} else {
 		/* If ATTR_CTIME isn't set, then ATTR_MTIME shouldn't be either. */
 		WARN_ON_ONCE(ia_valid & ATTR_MTIME);
@@ -352,19 +345,15 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode *inode,
 		inode->i_mode = mode;
 	}
 
-	if (is_mgtime(inode))
+	if (!(ia_valid & ATTR_DELEG) && is_mgtime(inode))
 		return setattr_copy_mgtime(inode, attr);
 
 	if (ia_valid & ATTR_ATIME)
 		inode_set_atime_to_ts(inode, attr->ia_atime);
 	if (ia_valid & ATTR_MTIME)
 		inode_set_mtime_to_ts(inode, attr->ia_mtime);
-	if (ia_valid & ATTR_CTIME) {
-		if (ia_valid & ATTR_DELEG)
-			inode_set_ctime_deleg(inode, attr->ia_ctime);
-		else
-			inode_set_ctime_to_ts(inode, attr->ia_ctime);
-	}
+	if (ia_valid & ATTR_CTIME)
+		inode_set_ctime_to_ts(inode, attr->ia_ctime);
 }
 EXPORT_SYMBOL(setattr_copy);
 
diff --git a/fs/inode.c b/fs/inode.c
index 99318b157a9a13b3dd8dad0f5f90951f08ef64de..f45054fe48b8a0339e60fd2aa17daaad5a7957e7 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2783,79 +2783,6 @@ struct timespec64 inode_set_ctime_current(struct inode *inode)
 }
 EXPORT_SYMBOL(inode_set_ctime_current);
 
-/**
- * inode_set_ctime_deleg - try to update the ctime on a delegated inode
- * @inode: inode to update
- * @update: timespec64 to set the ctime
- *
- * Attempt to atomically update the ctime on behalf of a delegation holder.
- *
- * The nfs server can call back the holder of a delegation to get updated
- * inode attributes, including the mtime. When updating the mtime, update
- * the ctime to a value at least equal to that.
- *
- * This can race with concurrent updates to the inode, in which
- * case the update is skipped.
- *
- * Note that this works even when multigrain timestamps are not enabled,
- * so it is used in either case.
- */
-struct timespec64 inode_set_ctime_deleg(struct inode *inode, struct timespec64 update)
-{
-	struct timespec64 now, cur_ts;
-	u32 cur, old;
-
-	/* pairs with try_cmpxchg below */
-	cur = smp_load_acquire(&inode->i_ctime_nsec);
-	cur_ts.tv_nsec = cur & ~I_CTIME_QUERIED;
-	cur_ts.tv_sec = inode->i_ctime_sec;
-
-	/* If the update is older than the existing value, skip it. */
-	if (timespec64_compare(&update, &cur_ts) <= 0)
-		return cur_ts;
-
-	ktime_get_coarse_real_ts64_mg(&now);
-
-	/* Clamp the update to "now" if it's in the future */
-	if (timespec64_compare(&update, &now) > 0)
-		update = now;
-
-	update = timestamp_truncate(update, inode);
-
-	/* No need to update if the values are already the same */
-	if (timespec64_equal(&update, &cur_ts))
-		return cur_ts;
-
-	/*
-	 * Try to swap the nsec value into place. If it fails, that means
-	 * it raced with an update due to a write or similar activity. That
-	 * stamp takes precedence, so just skip the update.
-	 */
-retry:
-	old = cur;
-	if (try_cmpxchg(&inode->i_ctime_nsec, &cur, update.tv_nsec)) {
-		inode->i_ctime_sec = update.tv_sec;
-		mgtime_counter_inc(mg_ctime_swaps);
-		return update;
-	}
-
-	/*
-	 * Was the change due to another task marking the old ctime QUERIED?
-	 *
-	 * If so, then retry the swap. This can only happen once since
-	 * the only way to clear I_CTIME_QUERIED is to stamp the inode
-	 * with a new ctime.
-	 */
-	if (!(old & I_CTIME_QUERIED) && (cur == (old | I_CTIME_QUERIED)))
-		goto retry;
-
-	/* Otherwise, it was a new timestamp. */
-	cur_ts.tv_sec = inode->i_ctime_sec;
-	cur_ts.tv_nsec = cur & ~I_CTIME_QUERIED;
-	return cur_ts;
-}
-EXPORT_SYMBOL(inode_set_ctime_deleg);
-
 /**
  * in_group_or_capable - check whether caller is CAP_FSETID privileged
  * @idmap:	idmap of the mount @inode was found from
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f18f45e88545c39716b917b1378fb7248367b41d..08f2d813dd40b5dd4fe07d9636e94252915d6235 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1657,8 +1657,6 @@ static inline bool fsuidgid_has_mapping(struct super_block *sb,
 
 struct timespec64 current_time(struct inode *inode);
 struct timespec64 inode_set_ctime_current(struct inode *inode);
-struct timespec64 inode_set_ctime_deleg(struct inode *inode,
-					struct timespec64 update);
 
 static inline time64_t inode_get_atime_sec(const struct inode *inode)
 {

-- 
2.50.1

Re: [PATCH v2 3/7] nfsd: use ATTR_CTIME_SET for delegated ctime updates

[Chuck Lever]

Hi Jeff -

Thanks again for your focus on getting this straightened out!


On 7/26/25 10:31 AM, Jeff Layton wrote:
> Ensure that notify_change() doesn't clobber a delegated ctime update
> with current_time() by setting ATTR_CTIME_SET for those updates.
> 
> Also, set the tv_nsec field the nfsd4_decode_fattr4 to the correct
> value.

I don't yet see the connection of the above tv_nsec fix to the other
changes in this patch. Wouldn't this be an independent fix?


> Don't bother setting the timestamps in cb_getattr_update_times() in the
> non-delegated case. notify_change() will do that itself.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

General comments:

I don't feel that any of the patches in this series need to be tagged
for stable, since there is already a Kconfig setting that defaults to
leaving timestamp delegation disabled. But I would like to see Fixes:
tags, where that makes sense?

Is this set on top of the set you posted a day or two ago with the new
trace point? Or does this set replace that one?


> ---
>  fs/nfsd/nfs4state.c | 6 +++---
>  fs/nfsd/nfs4xdr.c   | 5 +++--
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 88c347957da5b8f352be63f84f207d2225f81cb9..77eea2ad93cc07939f045fc4b983b1ac00d068b8 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -9167,7 +9167,6 @@ static bool set_cb_time(struct timespec64 *cb, const struct timespec64 *orig,
>  static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation *dp)
>  {
>  	struct inode *inode = d_inode(dentry);
> -	struct timespec64 now = current_time(inode);
>  	struct nfs4_cb_fattr *ncf = &dp->dl_cb_fattr;
>  	struct iattr attrs = { };
>  	int ret;
> @@ -9175,6 +9174,7 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
>  	if (deleg_attrs_deleg(dp->dl_type)) {
>  		struct timespec64 atime = inode_get_atime(inode);
>  		struct timespec64 mtime = inode_get_mtime(inode);
> +		struct timespec64 now = current_time(inode);
>  
>  		attrs.ia_atime = ncf->ncf_cb_atime;
>  		attrs.ia_mtime = ncf->ncf_cb_mtime;
> @@ -9183,12 +9183,12 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
>  			attrs.ia_valid |= ATTR_ATIME | ATTR_ATIME_SET;
>  
>  		if (set_cb_time(&attrs.ia_mtime, &mtime, &now)) {
> -			attrs.ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET;
> +			attrs.ia_valid |= ATTR_CTIME | ATTR_CTIME_SET |
> +					  ATTR_MTIME | ATTR_MTIME_SET;
>  			attrs.ia_ctime = attrs.ia_mtime;
>  		}
>  	} else {
>  		attrs.ia_valid |= ATTR_MTIME | ATTR_CTIME;
> -		attrs.ia_mtime = attrs.ia_ctime = now;
>  	}
>  
>  	if (!attrs.ia_valid)
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 8b68f74a8cf08c6aa1305a2a3093467656085e4a..c0a3c6a7c8bb70d62940115c3101e9f897401456 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -538,8 +538,9 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
>  		iattr->ia_mtime.tv_sec = modify.seconds;
>  		iattr->ia_mtime.tv_nsec = modify.nseconds;
>  		iattr->ia_ctime.tv_sec = modify.seconds;
> -		iattr->ia_ctime.tv_nsec = modify.seconds;
> -		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
> +		iattr->ia_ctime.tv_nsec = modify.nseconds;
> +		iattr->ia_valid |= ATTR_CTIME | ATTR_CTIME_SET |
> +				   ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
>  	}
>  
>  	/* request sanity: did attrlist4 contain the expected number of words? */
> 



-- 
Chuck Lever

Re: [PATCH v2 3/7] nfsd: use ATTR_CTIME_SET for delegated ctime updates

[Jeff Layton]

On Sat, 2025-07-26 at 14:48 -0400, Chuck Lever wrote:
> Hi Jeff -
> 
> Thanks again for your focus on getting this straightened out!
> 
> 
> On 7/26/25 10:31 AM, Jeff Layton wrote:
> > Ensure that notify_change() doesn't clobber a delegated ctime update
> > with current_time() by setting ATTR_CTIME_SET for those updates.
> > 
> > Also, set the tv_nsec field the nfsd4_decode_fattr4 to the correct
> > value.
> 
> I don't yet see the connection of the above tv_nsec fix to the other
> changes in this patch. Wouldn't this be an independent fix?
> 

I felt like they were related. Yes, the ia_ctime field is currently
being set wrong, but it's also being clobbered by notify_change(), so
it doesn't matter much. I can break this into a separate patch (with a
Fixes: tag) if you prefer though.

> > Don't bother setting the timestamps in cb_getattr_update_times() in the
> > non-delegated case. notify_change() will do that itself.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> 
> General comments:
> 
> I don't feel that any of the patches in this series need to be tagged
> for stable, since there is already a Kconfig setting that defaults to
> leaving timestamp delegation disabled. But I would like to see Fixes:
> tags, where that makes sense?
> 

I don't think any of these need to go to stable since this is still
under a non-default Kconfig option, and the main effect of the bug is
wonky timestamps. I should be able to add some Fixes: tags though.

> Is this set on top of the set you posted a day or two ago with the new
> trace point? Or does this set replace that one?
> 

This set should replace those.

> 
> > ---
> >  fs/nfsd/nfs4state.c | 6 +++---
> >  fs/nfsd/nfs4xdr.c   | 5 +++--
> >  2 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 88c347957da5b8f352be63f84f207d2225f81cb9..77eea2ad93cc07939f045fc4b983b1ac00d068b8 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -9167,7 +9167,6 @@ static bool set_cb_time(struct timespec64 *cb, const struct timespec64 *orig,
> >  static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation *dp)
> >  {
> >  	struct inode *inode = d_inode(dentry);
> > -	struct timespec64 now = current_time(inode);
> >  	struct nfs4_cb_fattr *ncf = &dp->dl_cb_fattr;
> >  	struct iattr attrs = { };
> >  	int ret;
> > @@ -9175,6 +9174,7 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
> >  	if (deleg_attrs_deleg(dp->dl_type)) {
> >  		struct timespec64 atime = inode_get_atime(inode);
> >  		struct timespec64 mtime = inode_get_mtime(inode);
> > +		struct timespec64 now = current_time(inode);
> >  
> >  		attrs.ia_atime = ncf->ncf_cb_atime;
> >  		attrs.ia_mtime = ncf->ncf_cb_mtime;
> > @@ -9183,12 +9183,12 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
> >  			attrs.ia_valid |= ATTR_ATIME | ATTR_ATIME_SET;
> >  
> >  		if (set_cb_time(&attrs.ia_mtime, &mtime, &now)) {
> > -			attrs.ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET;
> > +			attrs.ia_valid |= ATTR_CTIME | ATTR_CTIME_SET |
> > +					  ATTR_MTIME | ATTR_MTIME_SET;
> >  			attrs.ia_ctime = attrs.ia_mtime;
> >  		}
> >  	} else {
> >  		attrs.ia_valid |= ATTR_MTIME | ATTR_CTIME;
> > -		attrs.ia_mtime = attrs.ia_ctime = now;
> >  	}
> >  
> >  	if (!attrs.ia_valid)
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index 8b68f74a8cf08c6aa1305a2a3093467656085e4a..c0a3c6a7c8bb70d62940115c3101e9f897401456 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -538,8 +538,9 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
> >  		iattr->ia_mtime.tv_sec = modify.seconds;
> >  		iattr->ia_mtime.tv_nsec = modify.nseconds;
> >  		iattr->ia_ctime.tv_sec = modify.seconds;
> > -		iattr->ia_ctime.tv_nsec = modify.seconds;
> > -		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
> > +		iattr->ia_ctime.tv_nsec = modify.nseconds;
> > +		iattr->ia_valid |= ATTR_CTIME | ATTR_CTIME_SET |
> > +				   ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
> >  	}
> >  
> >  	/* request sanity: did attrlist4 contain the expected number of words? */
> > 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 3/7] nfsd: use ATTR_CTIME_SET for delegated ctime updates

[Chuck Lever]

On 7/26/25 3:03 PM, Jeff Layton wrote:
> On Sat, 2025-07-26 at 14:48 -0400, Chuck Lever wrote:
>> Hi Jeff -
>>
>> Thanks again for your focus on getting this straightened out!
>>
>>
>> On 7/26/25 10:31 AM, Jeff Layton wrote:
>>> Ensure that notify_change() doesn't clobber a delegated ctime update
>>> with current_time() by setting ATTR_CTIME_SET for those updates.
>>>
>>> Also, set the tv_nsec field the nfsd4_decode_fattr4 to the correct
>>> value.
>>
>> I don't yet see the connection of the above tv_nsec fix to the other
>> changes in this patch. Wouldn't this be an independent fix?
>>
> 
> I felt like they were related. Yes, the ia_ctime field is currently
> being set wrong, but it's also being clobbered by notify_change(), so
> it doesn't matter much. I can break this into a separate patch (with a
> Fixes: tag) if you prefer though.

Ah, got it, this patch exposes a latent bug. The usual thing to do is to
fix the latent bug in a preceding/pre-requisite patch, so that's my
preference.


>>> Don't bother setting the timestamps in cb_getattr_update_times() in the
>>> non-delegated case. notify_change() will do that itself.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>
>> General comments:
>>
>> I don't feel that any of the patches in this series need to be tagged
>> for stable, since there is already a Kconfig setting that defaults to
>> leaving timestamp delegation disabled. But I would like to see Fixes:
>> tags, where that makes sense?
>>
> 
> I don't think any of these need to go to stable since this is still
> under a non-default Kconfig option, and the main effect of the bug is
> wonky timestamps. I should be able to add some Fixes: tags though.
> 
>> Is this set on top of the set you posted a day or two ago with the new
>> trace point? Or does this set replace that one?
>>
> 
> This set should replace those.

I was confused because the trace point patch is missing, and dropping it
wasn't mentioned in the cover letter's Change log. NBD, thanks for
clarifying.

Since the bulk of these are NFSD changes, I volunteer to take v3 once
we have Acks from the VFS maintainers, as needed.


>>> ---
>>>  fs/nfsd/nfs4state.c | 6 +++---
>>>  fs/nfsd/nfs4xdr.c   | 5 +++--
>>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 88c347957da5b8f352be63f84f207d2225f81cb9..77eea2ad93cc07939f045fc4b983b1ac00d068b8 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -9167,7 +9167,6 @@ static bool set_cb_time(struct timespec64 *cb, const struct timespec64 *orig,
>>>  static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation *dp)
>>>  {
>>>  	struct inode *inode = d_inode(dentry);
>>> -	struct timespec64 now = current_time(inode);
>>>  	struct nfs4_cb_fattr *ncf = &dp->dl_cb_fattr;
>>>  	struct iattr attrs = { };
>>>  	int ret;
>>> @@ -9175,6 +9174,7 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
>>>  	if (deleg_attrs_deleg(dp->dl_type)) {
>>>  		struct timespec64 atime = inode_get_atime(inode);
>>>  		struct timespec64 mtime = inode_get_mtime(inode);
>>> +		struct timespec64 now = current_time(inode);
>>>  
>>>  		attrs.ia_atime = ncf->ncf_cb_atime;
>>>  		attrs.ia_mtime = ncf->ncf_cb_mtime;
>>> @@ -9183,12 +9183,12 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
>>>  			attrs.ia_valid |= ATTR_ATIME | ATTR_ATIME_SET;
>>>  
>>>  		if (set_cb_time(&attrs.ia_mtime, &mtime, &now)) {
>>> -			attrs.ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET;
>>> +			attrs.ia_valid |= ATTR_CTIME | ATTR_CTIME_SET |
>>> +					  ATTR_MTIME | ATTR_MTIME_SET;
>>>  			attrs.ia_ctime = attrs.ia_mtime;
>>>  		}
>>>  	} else {
>>>  		attrs.ia_valid |= ATTR_MTIME | ATTR_CTIME;
>>> -		attrs.ia_mtime = attrs.ia_ctime = now;
>>>  	}
>>>  
>>>  	if (!attrs.ia_valid)
>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>>> index 8b68f74a8cf08c6aa1305a2a3093467656085e4a..c0a3c6a7c8bb70d62940115c3101e9f897401456 100644
>>> --- a/fs/nfsd/nfs4xdr.c
>>> +++ b/fs/nfsd/nfs4xdr.c
>>> @@ -538,8 +538,9 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
>>>  		iattr->ia_mtime.tv_sec = modify.seconds;
>>>  		iattr->ia_mtime.tv_nsec = modify.nseconds;
>>>  		iattr->ia_ctime.tv_sec = modify.seconds;
>>> -		iattr->ia_ctime.tv_nsec = modify.seconds;
>>> -		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
>>> +		iattr->ia_ctime.tv_nsec = modify.nseconds;
>>> +		iattr->ia_valid |= ATTR_CTIME | ATTR_CTIME_SET |
>>> +				   ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
>>>  	}
>>>  
>>>  	/* request sanity: did attrlist4 contain the expected number of words? */
>>>
>>
>>
> 


-- 
Chuck Lever

Re: [PATCH v2 3/7] nfsd: use ATTR_CTIME_SET for delegated ctime updates

[Jeff Layton]

On Sat, 2025-07-26 at 15:57 -0400, Chuck Lever wrote:
> On 7/26/25 3:03 PM, Jeff Layton wrote:
> > On Sat, 2025-07-26 at 14:48 -0400, Chuck Lever wrote:
> > > Hi Jeff -
> > > 
> > > Thanks again for your focus on getting this straightened out!
> > > 
> > > 
> > > On 7/26/25 10:31 AM, Jeff Layton wrote:
> > > > Ensure that notify_change() doesn't clobber a delegated ctime update
> > > > with current_time() by setting ATTR_CTIME_SET for those updates.
> > > > 
> > > > Also, set the tv_nsec field the nfsd4_decode_fattr4 to the correct
> > > > value.
> > > 
> > > I don't yet see the connection of the above tv_nsec fix to the other
> > > changes in this patch. Wouldn't this be an independent fix?
> > > 
> > 
> > I felt like they were related. Yes, the ia_ctime field is currently
> > being set wrong, but it's also being clobbered by notify_change(), so
> > it doesn't matter much. I can break this into a separate patch (with a
> > Fixes: tag) if you prefer though.
> 
> Ah, got it, this patch exposes a latent bug. The usual thing to do is to
> fix the latent bug in a preceding/pre-requisite patch, so that's my
> preference.
> 

OK. I'll plan to send a v3 set.

> 
> > > > Don't bother setting the timestamps in cb_getattr_update_times() in the
> > > > non-delegated case. notify_change() will do that itself.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > 
> > > General comments:
> > > 
> > > I don't feel that any of the patches in this series need to be tagged
> > > for stable, since there is already a Kconfig setting that defaults to
> > > leaving timestamp delegation disabled. But I would like to see Fixes:
> > > tags, where that makes sense?
> > > 
> > 
> > I don't think any of these need to go to stable since this is still
> > under a non-default Kconfig option, and the main effect of the bug is
> > wonky timestamps. I should be able to add some Fixes: tags though.
> > 
> > > Is this set on top of the set you posted a day or two ago with the new
> > > trace point? Or does this set replace that one?
> > > 
> > 
> > This set should replace those.
> 
> I was confused because the trace point patch is missing, and dropping it
> wasn't mentioned in the cover letter's Change log. NBD, thanks for
> clarifying.
> 

Ahh sorry. The last patch drops the function to which I was adding the
tracepoint, so I dropped the tracepoint as well.

> Since the bulk of these are NFSD changes, I volunteer to take v3 once
> we have Acks from the VFS maintainers, as needed.
> 

Many thanks!

> 
> > > > ---
> > > >  fs/nfsd/nfs4state.c | 6 +++---
> > > >  fs/nfsd/nfs4xdr.c   | 5 +++--
> > > >  2 files changed, 6 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > index 88c347957da5b8f352be63f84f207d2225f81cb9..77eea2ad93cc07939f045fc4b983b1ac00d068b8 100644
> > > > --- a/fs/nfsd/nfs4state.c
> > > > +++ b/fs/nfsd/nfs4state.c
> > > > @@ -9167,7 +9167,6 @@ static bool set_cb_time(struct timespec64 *cb, const struct timespec64 *orig,
> > > >  static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation *dp)
> > > >  {
> > > >  	struct inode *inode = d_inode(dentry);
> > > > -	struct timespec64 now = current_time(inode);
> > > >  	struct nfs4_cb_fattr *ncf = &dp->dl_cb_fattr;
> > > >  	struct iattr attrs = { };
> > > >  	int ret;
> > > > @@ -9175,6 +9174,7 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
> > > >  	if (deleg_attrs_deleg(dp->dl_type)) {
> > > >  		struct timespec64 atime = inode_get_atime(inode);
> > > >  		struct timespec64 mtime = inode_get_mtime(inode);
> > > > +		struct timespec64 now = current_time(inode);
> > > >  
> > > >  		attrs.ia_atime = ncf->ncf_cb_atime;
> > > >  		attrs.ia_mtime = ncf->ncf_cb_mtime;
> > > > @@ -9183,12 +9183,12 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
> > > >  			attrs.ia_valid |= ATTR_ATIME | ATTR_ATIME_SET;
> > > >  
> > > >  		if (set_cb_time(&attrs.ia_mtime, &mtime, &now)) {
> > > > -			attrs.ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET;
> > > > +			attrs.ia_valid |= ATTR_CTIME | ATTR_CTIME_SET |
> > > > +					  ATTR_MTIME | ATTR_MTIME_SET;
> > > >  			attrs.ia_ctime = attrs.ia_mtime;
> > > >  		}
> > > >  	} else {
> > > >  		attrs.ia_valid |= ATTR_MTIME | ATTR_CTIME;
> > > > -		attrs.ia_mtime = attrs.ia_ctime = now;
> > > >  	}
> > > >  
> > > >  	if (!attrs.ia_valid)
> > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > > > index 8b68f74a8cf08c6aa1305a2a3093467656085e4a..c0a3c6a7c8bb70d62940115c3101e9f897401456 100644
> > > > --- a/fs/nfsd/nfs4xdr.c
> > > > +++ b/fs/nfsd/nfs4xdr.c
> > > > @@ -538,8 +538,9 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
> > > >  		iattr->ia_mtime.tv_sec = modify.seconds;
> > > >  		iattr->ia_mtime.tv_nsec = modify.nseconds;
> > > >  		iattr->ia_ctime.tv_sec = modify.seconds;
> > > > -		iattr->ia_ctime.tv_nsec = modify.seconds;
> > > > -		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
> > > > +		iattr->ia_ctime.tv_nsec = modify.nseconds;
> > > > +		iattr->ia_valid |= ATTR_CTIME | ATTR_CTIME_SET |
> > > > +				   ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
> > > >  	}
> > > >  
> > > >  	/* request sanity: did attrlist4 contain the expected number of words? */
> > > > 
> > > 
> > > 
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>