[PATCH v3 0/8] nfsd/vfs: fix handling of delegated timestamp updates

[PATCH v3 0/8] nfsd/vfs: fix handling of delegated timestamp updates

[Jeff Layton]

This patchset fixes the handling of delegated timestamps in nfsd.

This posting is basically identical to the last, aside from
splitting out one fix into a separate patch, and the addition of some
Fixes: tags.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v3:
- split out decoder fix into separate patch
- add Fixes: tags
- Link to v2: https://lore.kernel.org/r/20250726-nfsd-testing-v2-0-f45923db2fbb@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 (8):
      nfsd: fix assignment of ia_ctime.tv_nsec on delegated mtime update
      nfsd: ignore ATTR_DELEG when checking ia_valid before notify_change()
      vfs: add ATTR_CTIME_SET flag
      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 v3 1/8] nfsd: fix assignment of ia_ctime.tv_nsec on delegated mtime update

[Jeff Layton]

The ia_ctime.tv_nsec field should be set to modify.nseconds.

Fixes: 7e13f4f8d27d ("nfsd: handle delegated timestamps in SETATTR")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4xdr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 8b68f74a8cf08c6aa1305a2a3093467656085e4a..52033e2d603eb545dda781df5458da7d9805a373 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -538,7 +538,7 @@ 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_ctime.tv_nsec = modify.nseconds;
 		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
 	}
 

-- 
2.50.1

[PATCH v3 2/8] 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.

Fixes: 7e13f4f8d27d ("nfsd: handle delegated timestamps in SETATTR")
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 v3 3/8] 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 v3 4/8] 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.

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

Fixes: 7e13f4f8d27d ("nfsd: handle delegated timestamps in SETATTR")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 6 +++---
 fs/nfsd/nfs4xdr.c   | 3 ++-
 2 files changed, 5 insertions(+), 4 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 52033e2d603eb545dda781df5458da7d9805a373..c0a3c6a7c8bb70d62940115c3101e9f897401456 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -539,7 +539,8 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
 		iattr->ia_mtime.tv_nsec = modify.nseconds;
 		iattr->ia_ctime.tv_sec = modify.seconds;
 		iattr->ia_ctime.tv_nsec = modify.nseconds;
-		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
+		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 v3 5/8] 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/

Fixes: 7e13f4f8d27d ("nfsd: handle delegated timestamps in SETATTR")
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 v3 6/8] 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.

Fixes: 7e13f4f8d27d ("nfsd: handle delegated timestamps in SETATTR")
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 v3 7/8] 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.

Fixes: 6ae30d6eb26b ("nfsd: add support for delegated timestamps")
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 v3 8/8] 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 v3 0/8] nfsd/vfs: fix handling of delegated timestamp updates

[Chuck Lever]

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

On Sun, 27 Jul 2025 14:36:10 -0400, Jeff Layton wrote:
> This patchset fixes the handling of delegated timestamps in nfsd.
> 
> This posting is basically identical to the last, aside from
> splitting out one fix into a separate patch, and the addition of some
> Fixes: tags.
> 
> 
> [...]

Applied to nfsd-testing, thanks!

[1/8] nfsd: fix assignment of ia_ctime.tv_nsec on delegated mtime update
      commit: fe4f2bcb1a5ec8301af577b9373c80842212145c
[2/8] nfsd: ignore ATTR_DELEG when checking ia_valid before notify_change()
      commit: 4d619040969071a8e2fb51f66df52cc9fc25015f
[3/8] vfs: add ATTR_CTIME_SET flag
      commit: a8adc73b9ff670b77ff3e99b315a4f2c49123444
[4/8] nfsd: use ATTR_CTIME_SET for delegated ctime updates
      commit: 9c5d4468d49e0066682f473e0d4c908d904d350e
[5/8] nfsd: track original timestamps in nfs4_delegation
      commit: 3d0b3a6ab22cb233b9cc52872ba0ff2350eb9ba0
[6/8] nfsd: fix SETATTR updates for delegated timestamps
      commit: 1a64065d565d76942fd086c134de70cad3515887
[7/8] nfsd: fix timestamp updates in CB_GETATTR
      commit: 409cc8fd6365956ce0d0f14d20d1c59b4c05f5b2
[8/8] vfs: remove inode_set_ctime_deleg()
      commit: 5cca8d3d2fc34440fb73f5b8331d5228ef6d151a

--
Chuck Lever

Re: [PATCH v3 3/8] vfs: add ATTR_CTIME_SET flag

[NeilBrown]

On Mon, 28 Jul 2025, Jeff Layton wrote:
> 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.

I don't feel entirely comfortable with this.  ctime is a fallback for
"has anything changed" - mtime can be changed but ctime is always
reliable, controlled by VFS and FS.

Until now.

I know you aren't exposing this to user-space, but then not doing so
blocks user-space file servers from using this functionality.

I see that you also move vetting of the value out of vfs code and into
nfsd code.  I don't really understand why you did that.  Maybe nfsd has
more information about previous timestamps than the vfs has?

Anyway I would much prefer that ATTR_CTIME_SET could only change the
ctime value to something between the old ctime value and the current
time (inclusive).

Certainly nfsd might impose extra restrictions, but I think that basic
restriction should by in the VFS close to what ATTR_CTIME_SET is
honoured.  What way if someone else finds another use for it some day
they will have to work within the same restriction (or change it
explicitly and try to justify that change).

Lustre has the equivalent of ATTR_CTIME_SET (MFS_ATTR_CTIME_SET and
LA_CTIME) and would want to use it if the server-side code ever landed
upstream.  It appears to just assume the client sent a valid timestamp.
I would rather it were vetted by the VFS.

Thanks,
NeilBrown


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

Re: [PATCH v3 3/8] vfs: add ATTR_CTIME_SET flag

[Jeff Layton]

On Mon, 2025-07-28 at 10:04 +1000, NeilBrown wrote:
> On Mon, 28 Jul 2025, Jeff Layton wrote:
> > 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.
> 
> I don't feel entirely comfortable with this.  ctime is a fallback for
> "has anything changed" - mtime can be changed but ctime is always
> reliable, controlled by VFS and FS.
> 
> Until now.
> 

I know. I have many of the same reservations, but the specification is
pretty clear (now that I understand it better). I don't see a better
way to do this.

> I know you aren't exposing this to user-space, but then not doing so
> blocks user-space file servers from using this functionality.
> 
> I see that you also move vetting of the value out of vfs code and into
> nfsd code.  I don't really understand why you did that.  Maybe nfsd has
> more information about previous timestamps than the vfs has?
> 

Yes. We need to track the timestamps of the inode at the time that the
delegation was handed out. nfsd is (arguably) in a better position to
do this than the VFS is. Patch #5 adds this functionality.

> Anyway I would much prefer that ATTR_CTIME_SET could only change the
> ctime value to something between the old ctime value and the current
> time (inclusive).
> 

That will be a problem. What you're suggesting is the current status
quo with the delegated attrs code, and that behavior was the source of
the problems that we were seeing in the git regression testsuite.


When git checks out an object, it opens a file, writes to it and then
stats it so that it can later see whether it changed. If it gets a
WRITE_ATTRS_DELEG delegation, the client doesn't wait on writeback
before returning from that stat().

Then later, we go to do writeback. The mtime and ctime on the server
get set to the server's local time (which is later than the time that
git has recorded). Finally, the client does the SETATTR+DELEGRETURN and
tries to set the timestamps to the same times that git has recorded,
but those times are too early vs. the current timestamps on the file
and they get ignored (in accordance with the spec).

This was the source of my confusion with the spec. When it says
"original time", it means the timestamps at the time that the
delegation was created, but I interpreted it the same way you did.

Unfortunately, if we want to do this, then we have to allow nfsd to set
the ctime to a time earlier than the current ctime on the inode. I too
have some reservations with this. This means that applications on the
server may see the ctime go backward, which I really do not like. 

In practice though, if there is an outstanding delegation then those
applications can't do anything other than stat() the file without
causing it to be recalled. They can't have the file open at the time,
and can't do any directory operations that involve it. Given that, I
think the ctime rollbacks are "mostly harmless". 

Moving these checks into the VFS would be pretty ugly, unless we want
to tightly integrate the setattr and lease handling code. nfsd is just
in a much better position to track and vet this info than the VFS.

> Certainly nfsd might impose extra restrictions, but I think that basic
> restriction should by in the VFS close to what ATTR_CTIME_SET is
> honoured.  What way if someone else finds another use for it some day
> they will have to work within the same restriction (or change it
> explicitly and try to justify that change).
> 
> Lustre has the equivalent of ATTR_CTIME_SET (MFS_ATTR_CTIME_SET and
> LA_CTIME) and would want to use it if the server-side code ever landed
> upstream.  It appears to just assume the client sent a valid timestamp.
> I would rather it were vetted by the VFS.
>

Interesting. I don't think they have any immediate plans to upstream
the server (the priority is the client), but having this functionality
in the VFS would make it easier to integrate.

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

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3 3/8] vfs: add ATTR_CTIME_SET flag

[NeilBrown]

On Mon, 28 Jul 2025, Jeff Layton wrote:
> On Mon, 2025-07-28 at 10:04 +1000, NeilBrown wrote:
> > On Mon, 28 Jul 2025, Jeff Layton wrote:
> > > 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.
> > 
> > I don't feel entirely comfortable with this.  ctime is a fallback for
> > "has anything changed" - mtime can be changed but ctime is always
> > reliable, controlled by VFS and FS.
> > 
> > Until now.
> > 
> 
> I know. I have many of the same reservations, but the specification is
> pretty clear (now that I understand it better). I don't see a better
> way to do this.
> 
> > I know you aren't exposing this to user-space, but then not doing so
> > blocks user-space file servers from using this functionality.
> > 
> > I see that you also move vetting of the value out of vfs code and into
> > nfsd code.  I don't really understand why you did that.  Maybe nfsd has
> > more information about previous timestamps than the vfs has?
> > 
> 
> Yes. We need to track the timestamps of the inode at the time that the
> delegation was handed out. nfsd is (arguably) in a better position to
> do this than the VFS is. Patch #5 adds this functionality.
> 
> > Anyway I would much prefer that ATTR_CTIME_SET could only change the
> > ctime value to something between the old ctime value and the current
> > time (inclusive).
> > 
> 
> That will be a problem. What you're suggesting is the current status
> quo with the delegated attrs code, and that behavior was the source of
> the problems that we were seeing in the git regression testsuite.
> 
> 
> When git checks out an object, it opens a file, writes to it and then
> stats it so that it can later see whether it changed. If it gets a
> WRITE_ATTRS_DELEG delegation, the client doesn't wait on writeback
> before returning from that stat().
> 
> Then later, we go to do writeback. The mtime and ctime on the server
> get set to the server's local time (which is later than the time that
> git has recorded). Finally, the client does the SETATTR+DELEGRETURN and
> tries to set the timestamps to the same times that git has recorded,
> but those times are too early vs. the current timestamps on the file
> and they get ignored (in accordance with the spec).
> 
> This was the source of my confusion with the spec. When it says
> "original time", it means the timestamps at the time that the
> delegation was created, but I interpreted it the same way you did.
> 
> Unfortunately, if we want to do this, then we have to allow nfsd to set
> the ctime to a time earlier than the current ctime on the inode. I too
> have some reservations with this. This means that applications on the
> server may see the ctime go backward, which I really do not like. 

An alternate approach would be to allow the writeback through a
delegation to skip the mtime/ctime update, possibly making use of
FMODE_NOCMTIME.

It would be nice to have some mechanism in the VFS to ensure there was
an ATTR_CTIME_SET request on any file which had made use of
FMODE_NOCMTIME before that file was closed, else the times would be set
to the time of the close.  That wouldn't be entirely straight forward,
but should be manageable.  (I would allow some way to avoid the ctime
update on close so XFS_IOC_OPENBY_HANDLE could still be supported, but
it would need to be explicit somewhere).

While FMODE_NOCMTIME also distorts the meaning of ctime, I think it is
better than making it too easy for ctime to go backwards.

How would you feel about that approach?

> 
> Interesting. I don't think they have any immediate plans to upstream
> the server (the priority is the client), but having this functionality
> in the VFS would make it easier to integrate.

I think that splitting the client from the server is a non-trivial task
that brings no benefit to anyone.  Any chance I get I advocate
upstreaming both at once.

Thanks,
NeilBrown

Re: [PATCH v3 3/8] vfs: add ATTR_CTIME_SET flag

[Jeff Layton]

On Mon, 2025-07-28 at 11:51 +1000, NeilBrown wrote:
> On Mon, 28 Jul 2025, Jeff Layton wrote:
> > On Mon, 2025-07-28 at 10:04 +1000, NeilBrown wrote:
> > > On Mon, 28 Jul 2025, Jeff Layton wrote:
> > > > 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.
> > > 
> > > I don't feel entirely comfortable with this.  ctime is a fallback for
> > > "has anything changed" - mtime can be changed but ctime is always
> > > reliable, controlled by VFS and FS.
> > > 
> > > Until now.
> > > 
> > 
> > I know. I have many of the same reservations, but the specification is
> > pretty clear (now that I understand it better). I don't see a better
> > way to do this.
> > 
> > > I know you aren't exposing this to user-space, but then not doing so
> > > blocks user-space file servers from using this functionality.
> > > 
> > > I see that you also move vetting of the value out of vfs code and into
> > > nfsd code.  I don't really understand why you did that.  Maybe nfsd has
> > > more information about previous timestamps than the vfs has?
> > > 
> > 
> > Yes. We need to track the timestamps of the inode at the time that the
> > delegation was handed out. nfsd is (arguably) in a better position to
> > do this than the VFS is. Patch #5 adds this functionality.
> > 
> > > Anyway I would much prefer that ATTR_CTIME_SET could only change the
> > > ctime value to something between the old ctime value and the current
> > > time (inclusive).
> > > 
> > 
> > That will be a problem. What you're suggesting is the current status
> > quo with the delegated attrs code, and that behavior was the source of
> > the problems that we were seeing in the git regression testsuite.
> > 
> > 
> > When git checks out an object, it opens a file, writes to it and then
> > stats it so that it can later see whether it changed. If it gets a
> > WRITE_ATTRS_DELEG delegation, the client doesn't wait on writeback
> > before returning from that stat().
> > 
> > Then later, we go to do writeback. The mtime and ctime on the server
> > get set to the server's local time (which is later than the time that
> > git has recorded). Finally, the client does the SETATTR+DELEGRETURN and
> > tries to set the timestamps to the same times that git has recorded,
> > but those times are too early vs. the current timestamps on the file
> > and they get ignored (in accordance with the spec).
> > 
> > This was the source of my confusion with the spec. When it says
> > "original time", it means the timestamps at the time that the
> > delegation was created, but I interpreted it the same way you did.
> > 
> > Unfortunately, if we want to do this, then we have to allow nfsd to set
> > the ctime to a time earlier than the current ctime on the inode. I too
> > have some reservations with this. This means that applications on the
> > server may see the ctime go backward, which I really do not like. 
> 
> An alternate approach would be to allow the writeback through a
> delegation to skip the mtime/ctime update, possibly making use of
> FMODE_NOCMTIME.
> 
> It would be nice to have some mechanism in the VFS to ensure there was
> an ATTR_CTIME_SET request on any file which had made use of
> FMODE_NOCMTIME before that file was closed, else the times would be set
> to the time of the close.  That wouldn't be entirely straight forward,
> but should be manageable.  (I would allow some way to avoid the ctime
> update on close so XFS_IOC_OPENBY_HANDLE could still be supported, but
> it would need to be explicit somewhere).
> 
> While FMODE_NOCMTIME also distorts the meaning of ctime, I think it is
> better than making it too easy for ctime to go backwards.
> 
> How would you feel about that approach?
> 

I do like the idea of "freezing" timestamp updates until the delegation
is returned. Doing a bit of git-archaeology shows that the NOCMTIME
flag was added here:

commit 4d4be482a4d78ca906f45e99fd9fdb91e907f5ad
Author: Christoph Hellwig <hch@infradead.org>
Date:   Tue Dec 9 04:47:33 2008 -0500

    [XFS] add a FMODE flag to make XFS invisible I/O less hacky
    
    XFS has a mode called invisble I/O that doesn't update any of the
    timestamps.  It's used for HSM-style applications and exposed through
    the nasty open by handle ioctl.
    
    Instead of doing directly assignment of file operations that set an
    internal flag for it add a new FMODE_NOCMTIME flag that we can check
    in the normal file operations.
    
    (addition of the generic VFS flag has been ACKed by Al as an interims
     solution)
    
    Signed-off-by: Christoph Hellwig <hch@lst.de>
    Signed-off-by: Lachlan McIlroy <lachlan@sgi.com>

Delegated timestamps seems like a similar enough use-case that we can
probably make this work.

The main catch here is that there is no guarantee that the client will
ever follow up with a SETATTR, so (like you mentioned), we'd need a
mechanism to ensure that the cmtime can be updated on close, if the
file is ever written while the delegation is in force and the final
SETATTR never comes in.

I'll do a bit of research an get back to you on whether this is
feasible. Thanks for the suggestion!
-- 
Jeff Layton <jlayton@kernel.org>