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

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

[Jeff Layton]

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>
---
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      | 43 ++++++++++++++++++++++++++++++++++++++++++-
 fs/nfs/nfs42proc.c  | 12 +++++++++---
 3 files changed, 53 insertions(+), 4 deletions(-)
---
base-commit: d692f252c52b8d1b4bc7481554f314df31ed8249
change-id: 20261106-delegts-nfs-25dafe47b434

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

[PATCH 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.

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

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index e0a2e92952af..e990f353b7d0 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -629,6 +629,25 @@ 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_ctime_after(struct inode *inode, const struct timespec64 *old)
+{
+	struct timespec64 now = inode_get_ctime(inode);
+
+	if (timespec64_compare(&now, old) <= 0) {
+		now = *old;
+		timespec64_add_ns(&now, 1);
+		inode_set_ctime_to_ts(inode, now);
+	}
+	return now;
+}
+
 static void nfs_set_timestamps_to_ts(struct inode *inode, struct iattr *attr)
 {
 	unsigned int cache_flags = 0;
@@ -639,7 +658,8 @@ static void nfs_set_timestamps_to_ts(struct inode *inode, struct iattr *attr)
 		struct timespec64 now;
 		bool updated = false;
 
-		now = inode_set_ctime_current(inode);
+		inode_set_ctime_current(inode);
+		now = nfs_delegated_ctime_after(inode, &ctime);
 		if (!timespec64_equal(&now, &ctime))
 			updated = true;
 
@@ -665,7 +685,10 @@ static void nfs_update_atime(struct inode *inode)
 
 static void nfs_update_mtime(struct inode *inode)
 {
+	struct timespec64 ctime = inode_get_ctime(inode);
+
 	inode_update_time(inode, FS_UPD_CMTIME, 0);
+	inode_set_mtime_to_ts(inode, nfs_delegated_ctime_after(inode, &ctime));
 	NFS_I(inode)->cache_validity &=
 		~(NFS_INO_INVALID_CTIME | NFS_INO_INVALID_MTIME);
 }

-- 
2.56.0

[PATCH 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.

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

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index e990f353b7d0..8b86d93f8101 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -648,6 +648,21 @@ nfs_delegated_ctime_after(struct inode *inode, const struct timespec64 *old)
 	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;
@@ -860,6 +875,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 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, and the CLONE reply's attributes are then
filtered out. 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.

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.

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      |  5 +++--
 fs/nfs/nfs42proc.c  | 12 +++++++++---
 3 files changed, 14 insertions(+), 5 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 8b86d93f8101..0175e7719ab4 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -653,8 +653,8 @@ nfs_delegated_ctime_after(struct inode *inode, const struct timespec64 *old)
  * 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);
 
@@ -662,6 +662,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)
 {
diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
index ab86246fc364..3ab36730688e 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,6 +406,9 @@ 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);
 	spin_unlock(&inode->i_lock);
 	nfs_update_delegated_mtime(inode);
 }
@@ -506,7 +510,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,7 +1316,8 @@ 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);
+		nfs42_copy_dest_done(dst_f, dst_offset, count, oldsize_dst,
+				     res.dst_fattr);
 		status = nfs_post_op_update_inode(dst_inode, res.dst_fattr);
 	}
 

-- 
2.56.0