[PATCH v2 0/3] NFS: fixes for delegated timestamps

[PATCH v2 0/3] NFS: fixes for delegated timestamps

[Jeff Layton]

This version fixes some issues that Sashiko pointed out.

These are some client-side fixes for problems that I was seeing with
delegated timestamps. Mostly this was evident in the git regression
testsuite (which is particularly sensitive to timestamp issues).

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v2:
- "don't let delegated c/mtime stamps go backward": compute the stamp
  before storing it, so lockless readers never see the coarse time
- "stamp after the server's ctime when truncating under delegated times":
  stamp before nfs_vmtruncate() drops i_lock
- "stamp after the server's ctime following a CLONE":
  - stamp under i_lock
  - apply the reply with nfs_refresh_inode() under delegated times, so
    its ctime doesn't overwrite the stamp
- Link to v1: https://lore.kernel.org/r/20261106-delegts-nfs-v1-0-1d5c5e364a07@kernel.org

---
Jeff Layton (3):
      NFS: don't let delegated c/mtime stamps go backward
      NFS: stamp after the server's ctime when truncating under delegated times
      NFS: stamp after the server's ctime following a CLONE

 fs/nfs/delegation.h |  2 ++
 fs/nfs/inode.c      | 65 ++++++++++++++++++++++++++++++++++++++++-------------
 fs/nfs/nfs42proc.c  | 25 ++++++++++++++++-----
 3 files changed, 72 insertions(+), 20 deletions(-)
---
base-commit: 5e810a57a1cf2fc785d89e711ff40f1112f6404c
change-id: 20261106-delegts-nfs-25dafe47b434

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

[PATCH v2 1/3] NFS: don't let delegated c/mtime stamps go backward

[Jeff Layton]

With a timestamp delegation the client stamps c/mtime from its own
coarse clock. That can land before the server's fine-grained ctime the
client cached from the OPEN or SETATTR reply, e.g. on a WRITE right
after an exclusive create. The server then either rejects the times
(they don't pass dl_ctime) or can't apply the ctime, since
inode_set_ctime_deleg() never moves ctime backward. Either way the
c/mtime the client reported while holding the delegation changes
after DELEGRETURN; git t1092 over NFSv4.2 sees a stat-dirty file.

When a delegated stamp isn't after the cached ctime, use the cached
ctime + 1ns instead. Compute the stamp before storing it, so lockless
readers (getattr, CB_GETATTR) never see the trailing coarse time.

Fixes: e12912d94137 ("NFSv4: Add support for delegated atime and mtime attributes")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfs/inode.c | 40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 9839087cf373..98d8a34fab9d 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -629,25 +629,33 @@ nfs_fattr_fixup_delegated(struct inode *inode, struct nfs_fattr *fattr)
 	}
 }
 
+/*
+ * With delegated timestamps, the client's coarse clock can trail the
+ * server's fine-grained ctime cached from an OPEN or SETATTR reply. The
+ * server won't move ctime backward when the times are returned, so a
+ * new stamp must land after the cached ctime.
+ */
+static struct timespec64 nfs_delegated_stamp(struct inode *inode)
+{
+	struct timespec64 ctime = inode_get_ctime(inode);
+	struct timespec64 now = current_time(inode);
+
+	if (timespec64_compare(&now, &ctime) <= 0) {
+		now = ctime;
+		timespec64_add_ns(&now, 1);
+	}
+	return now;
+}
+
 static void nfs_set_timestamps_to_ts(struct inode *inode, struct iattr *attr)
 {
 	unsigned int cache_flags = 0;
 
 	if (attr->ia_valid & ATTR_MTIME_SET) {
-		struct timespec64 ctime = inode_get_ctime(inode);
-		struct timespec64 mtime = inode_get_mtime(inode);
-		struct timespec64 now;
-		bool updated = false;
-
-		now = inode_set_ctime_current(inode);
-		if (!timespec64_equal(&now, &ctime))
-			updated = true;
-
+		/* The stamp is always after the old ctime */
+		inode_set_ctime_to_ts(inode, nfs_delegated_stamp(inode));
 		inode_set_mtime_to_ts(inode, attr->ia_mtime);
-		if (!timespec64_equal(&now, &mtime))
-			updated = true;
-
-		inode_maybe_inc_iversion(inode, updated);
+		inode_maybe_inc_iversion(inode, true);
 		cache_flags |= NFS_INO_INVALID_CTIME | NFS_INO_INVALID_MTIME;
 	}
 	if (attr->ia_valid & ATTR_ATIME) {
@@ -665,7 +673,11 @@ static void nfs_update_atime(struct inode *inode)
 
 static void nfs_update_mtime(struct inode *inode)
 {
-	inode_update_time(inode, FS_UPD_CMTIME, 0);
+	struct timespec64 now = nfs_delegated_stamp(inode);
+
+	/* Not inode_update_time(): lockless readers would see its stamp */
+	inode_set_ctime_to_ts(inode, now);
+	inode_set_mtime_to_ts(inode, now);
 	NFS_I(inode)->cache_validity &=
 		~(NFS_INO_INVALID_CTIME | NFS_INO_INVALID_MTIME);
 }

-- 
2.56.0

[PATCH v2 2/3] NFS: stamp after the server's ctime when truncating under delegated times

[Jeff Layton]

A size-changing SETATTR always has the server stamp c/mtime
(__nfsd_setattr(), per RFC 5661 section 18.30.4). With delegated
timestamps the client discards the reply's ctime and stamps its own in
nfs_vmtruncate(), clamped only against its cached ctime. A coarse client
stamp can still land before the server's, which then won't move ctime
back when the holder returns its times, so ctime changes after
DELEGRETURN.

- Raise the cached ctime to the reply's before nfs_vmtruncate() stamps,
  so the delegated stamp lands after the server's.
- Stamp before nfs_vmtruncate() drops i_lock for truncate_pagecache(),
  not after. A delegation return starting in that window made the stamp
  a no-op, and DELEGRETURN sent the pre-truncate mtime, which the server
  could apply over its own truncate stamp.

Fixes: e12912d94137 ("NFSv4: Add support for delegated atime and mtime attributes")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfs/inode.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 98d8a34fab9d..270d8bb46110 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -647,6 +647,21 @@ static struct timespec64 nfs_delegated_stamp(struct inode *inode)
 	return now;
 }
 
+/*
+ * The server stamped c/mtime itself (e.g. for a size change). Raise the
+ * cached ctime to the server's so that the next delegated stamp lands
+ * after it.
+ */
+static void nfs_delegated_ctime_raise(struct inode *inode,
+				      const struct nfs_fattr *fattr)
+{
+	struct timespec64 ctime = inode_get_ctime(inode);
+
+	if ((fattr->valid & NFS_ATTR_FATTR_CTIME) &&
+	    timespec64_compare(&fattr->ctime, &ctime) > 0)
+		inode_set_ctime_to_ts(inode, fattr->ctime);
+}
+
 static void nfs_set_timestamps_to_ts(struct inode *inode, struct iattr *attr)
 {
 	unsigned int cache_flags = 0;
@@ -820,10 +835,14 @@ static int nfs_vmtruncate(struct inode * inode, loff_t offset)
 		nfs_ooo_clear(NFS_I(inode));
 	}
 	NFS_I(inode)->cache_validity &= ~NFS_INO_INVALID_SIZE;
+	/*
+	 * Stamp in the same i_lock section that checked the delegation and
+	 * raised ctime, before a delegation return can start.
+	 */
+	nfs_update_delegated_mtime_locked(inode);
 
 	spin_unlock(&inode->i_lock);
 	truncate_pagecache(inode, offset);
-	nfs_update_delegated_mtime_locked(inode);
 	spin_lock(&inode->i_lock);
 out:
 	return err;
@@ -849,6 +868,8 @@ void nfs_setattr_update_inode(struct inode *inode, struct iattr *attr,
 	if ((attr->ia_valid & ATTR_SIZE) != 0) {
 		if (!nfs_have_delegated_mtime(inode))
 			nfs_set_cache_invalid(inode, NFS_INO_INVALID_MTIME);
+		else
+			nfs_delegated_ctime_raise(inode, fattr);
 		nfs_set_cache_invalid(inode, NFS_INO_INVALID_BLOCKS);
 		nfs_inc_stats(inode, NFSIOS_SETATTRTRUNC);
 		nfs_vmtruncate(inode, attr->ia_size);

-- 
2.56.0

[PATCH v2 3/3] NFS: stamp after the server's ctime following a CLONE

[Jeff Layton]

nfs42_copy_dest_done() stamps the client's own c/mtime when it holds
delegated timestamps. A server that stamps c/mtime on CLONE (as nfsd did
before "nfsd: mark the delegation written on COPY/CLONE instead of
stamping c/mtime", and as btrfs did before "btrfs: honor FMODE_NOCMTIME
when cloning") can end up with a ctime newer than the client's coarse
stamp, which it won't move back when the holder returns its times.

The stamp didn't survive anyway: nfs_post_op_update_inode() then marks
ctime invalid, so nfs_fattr_fixup_delegated() keeps the reply's ctime
and nfs_update_inode() writes it over the stamp. Against a server that
no longer stamps on CLONE, the client's ctime goes back to its old value
until DELEGRETURN.

- Pass the CLONE reply's attributes to nfs42_copy_dest_done() and raise
  the cached ctime to the server's before stamping. COPY returns no
  attributes, so it passes NULL.
- Stamp before dropping i_lock, so a concurrent refresh can't take the
  server's times while ctime and mtime are marked invalid.
- Under delegated timestamps, apply the reply with nfs_refresh_inode(),
  which doesn't mark ctime invalid.

Fixes: 6e7daa3dad29 ("NFSv4.2: fix CLONE/COPY attrs in presence of delegated attributes")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfs/delegation.h |  2 ++
 fs/nfs/inode.c      |  6 ++++--
 fs/nfs/nfs42proc.c  | 25 ++++++++++++++++++++-----
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
index fba4699952b8..cd6142939d34 100644
--- a/fs/nfs/delegation.h
+++ b/fs/nfs/delegation.h
@@ -95,6 +95,8 @@ void nfs4_inode_make_writeable(struct inode *inode);
 void nfs_update_delegated_atime(struct inode *inode);
 void nfs_update_delegated_mtime(struct inode *inode);
 void nfs_update_delegated_mtime_locked(struct inode *inode);
+void nfs_delegated_ctime_raise(struct inode *inode,
+			       const struct nfs_fattr *fattr);
 
 static inline int nfs_have_read_or_write_delegation(struct inode *inode)
 {
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 270d8bb46110..5a682f74d7d1 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -652,8 +652,8 @@ static struct timespec64 nfs_delegated_stamp(struct inode *inode)
  * cached ctime to the server's so that the next delegated stamp lands
  * after it.
  */
-static void nfs_delegated_ctime_raise(struct inode *inode,
-				      const struct nfs_fattr *fattr)
+void nfs_delegated_ctime_raise(struct inode *inode,
+			       const struct nfs_fattr *fattr)
 {
 	struct timespec64 ctime = inode_get_ctime(inode);
 
@@ -661,6 +661,7 @@ static void nfs_delegated_ctime_raise(struct inode *inode,
 	    timespec64_compare(&fattr->ctime, &ctime) > 0)
 		inode_set_ctime_to_ts(inode, fattr->ctime);
 }
+EXPORT_SYMBOL_GPL(nfs_delegated_ctime_raise);
 
 static void nfs_set_timestamps_to_ts(struct inode *inode, struct iattr *attr)
 {
@@ -711,6 +712,7 @@ void nfs_update_delegated_mtime_locked(struct inode *inode)
 	    nfs_have_directory_delegation(inode))
 		nfs_update_mtime(inode);
 }
+EXPORT_SYMBOL_GPL(nfs_update_delegated_mtime_locked);
 
 void nfs_update_delegated_mtime(struct inode *inode)
 {
diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
index ab86246fc364..09b4a6ecad88 100644
--- a/fs/nfs/nfs42proc.c
+++ b/fs/nfs/nfs42proc.c
@@ -380,6 +380,7 @@ static int process_copy_commit(struct file *dst, loff_t pos_dst,
  * @pos: destination offset
  * @len: copy length
  * @oldsize: length of the file prior to clone/copy
+ * @fattr: destination attributes from the reply, or NULL
  *
  * Punch a hole in the inode page cache, so that the NFS client will
  * know to retrieve new data.
@@ -387,7 +388,7 @@ static int process_copy_commit(struct file *dst, loff_t pos_dst,
  * invalid cached values for change attribute, ctime, mtime and space used.
  */
 static void nfs42_copy_dest_done(struct file *file, loff_t pos, loff_t len,
-				 loff_t oldsize)
+				 loff_t oldsize, const struct nfs_fattr *fattr)
 {
 	struct inode *inode = file_inode(file);
 	struct address_space *mapping = file->f_mapping;
@@ -405,8 +406,12 @@ static void nfs42_copy_dest_done(struct file *file, loff_t pos, loff_t len,
 					     NFS_INO_INVALID_CTIME |
 					     NFS_INO_INVALID_MTIME |
 					     NFS_INO_INVALID_BLOCKS);
+	/* The server may have stamped c/mtime; stamp after it */
+	if (fattr && nfs_have_delegated_mtime(inode))
+		nfs_delegated_ctime_raise(inode, fattr);
+	/* Before a refresh can see the times invalid and take the server's */
+	nfs_update_delegated_mtime_locked(inode);
 	spin_unlock(&inode->i_lock);
-	nfs_update_delegated_mtime(inode);
 }
 
 static ssize_t _nfs42_proc_copy(struct file *src,
@@ -506,7 +511,8 @@ static ssize_t _nfs42_proc_copy(struct file *src,
 			goto out;
 	}
 
-	nfs42_copy_dest_done(dst, pos_dst, res->write_res.count, oldsize_dst);
+	nfs42_copy_dest_done(dst, pos_dst, res->write_res.count, oldsize_dst,
+			     NULL);
 	nfs_invalidate_atime(src_inode);
 	status = res->write_res.count;
 out:
@@ -1311,8 +1317,17 @@ static int _nfs42_proc_clone(struct rpc_message *msg, struct file *src_f,
 		/* a zero-length count means clone to EOF in src */
 		if (count == 0 && res.dst_fattr->valid & NFS_ATTR_FATTR_SIZE)
 			count = nfs_size_to_loff_t(res.dst_fattr->size) - dst_offset;
-		nfs42_copy_dest_done(dst_f, dst_offset, count, oldsize_dst);
-		status = nfs_post_op_update_inode(dst_inode, res.dst_fattr);
+		nfs42_copy_dest_done(dst_f, dst_offset, count, oldsize_dst,
+				     res.dst_fattr);
+		/*
+		 * nfs_post_op_update_inode() would mark ctime invalid, so the
+		 * reply's ctime would overwrite the delegated stamp.
+		 */
+		if (nfs_have_delegated_mtime(dst_inode))
+			status = nfs_refresh_inode(dst_inode, res.dst_fattr);
+		else
+			status = nfs_post_op_update_inode(dst_inode,
+							  res.dst_fattr);
 	}
 
 	kfree(res.dst_fattr);

-- 
2.56.0