[PATCH v3 3/8] nfsd: don't stamp over an applied delegated mtime at DELEGRETURN

[Jeff Layton <jlayton@kernel.org>]

nfsd4_finalize_deleg_timestamps() stamps a/m/ctime to "now" at
DELEGRETURN when the file was written and dl_setattr is clear. But
dl_setattr is only set when both the mtime and ctime vets pass:

- nfsd4_vet_deleg_attrs() applies the holder's mtime but leaves
  dl_setattr clear if that mtime doesn't pass dl_ctime. A client mtime a
  fraction of a millisecond behind the server's ctime at grant is
  enough.
- cb_getattr_update_times() applies the holder's mtime but never sets
  dl_setattr.

Either way, DELEGRETURN then overwrites the mtime the holder supplied
with the return time. Seen as git t1092 failures over NFSv4.2: an
untouched file's c/mtime jump ~25s after git recorded them.

- Set dl_setattr whenever the holder's mtime is applied. Test it under
  inode_lock at DELEGRETURN, so a racing CB_GETATTR reply's mtime isn't
  stamped over.
- If that mtime doesn't pass dl_ctime, set ctime to the current time
  instead of leaving it at its grant value. DELEGRETURN no longer
  stamps it, and where the change attribute derives from ctime (no
  i_version), it must still move across the holder's writes. dl_ctime
  is recorded from the inode, so later vetting sees that ctime. RFC 9754
  specifies the change time only when the mtime exceeds it; RFC 8881
  section 10.4.3 says the server SHOULD keep time_metadata and change
  from going backward.
- Have SETATTR and CB_GETATTR share nfsd4_vet_deleg_attrs(), so their
  vetting can't diverge again.

The client fix "NFS: don't let delegated c/mtime stamps go backward"
makes the ctime case rare.

Fixes: e5e9b24ab8fa ("nfsd: freeze c/mtime updates with outstanding WRITE_ATTRS delegation")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 82 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 35 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index a8966d1f786f..35712e129b7a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1463,6 +1463,12 @@ static void put_deleg_file(struct nfs4_file *fp)
 
 static void nfsd4_finalize_deleg_timestamps(struct nfs4_delegation *dp, struct file *f)
 {
+	struct inode *inode = file_inode(f);
+	struct iattr attr = {
+		.ia_valid = ATTR_ATIME | ATTR_CTIME | ATTR_MTIME | ATTR_DELEG,
+	};
+	int ret;
+
 	/* don't do anything if FMODE_NOCMTIME isn't set */
 	if ((READ_ONCE(f->f_mode) & FMODE_NOCMTIME) == 0)
 		return;
@@ -1475,12 +1481,22 @@ static void nfsd4_finalize_deleg_timestamps(struct nfs4_delegation *dp, struct f
 	if (!READ_ONCE(dp->dl_written))
 		return;
 
+	/* A racing CB_GETATTR reply sets dl_setattr under inode_lock */
+	inode_lock(inode);
 	/* did it get a setattr for the timestamps at some point? */
-	if (dp->dl_setattr)
+	if (dp->dl_setattr) {
+		inode_unlock(inode);
 		return;
+	}
 
 	/* Stamp everything to "now" */
-	nfsd_update_cmtime_attr(f, ATTR_ATIME);
+	ret = notify_change(&nop_mnt_idmap, f->f_path.dentry, &attr, NULL);
+	inode_unlock(inode);
+	if (ret)
+		pr_notice_ratelimited("nfsd: Unable to update timestamps on inode %02x:%02x:%llu: %d\n",
+				      MAJOR(inode->i_sb->s_dev),
+				      MINOR(inode->i_sb->s_dev),
+				      inode->i_ino, ret);
 }
 
 static void nfs4_unlock_deleg_lease(struct nfs4_delegation *dp)
@@ -10720,14 +10736,16 @@ bool nfsd4_vet_deleg_time(struct timespec64 *req, const struct timespec64 *orig,
 }
 
 /**
- * nfsd4_vet_deleg_attrs - vet the delegated timestamps in a SETATTR
- * @iattr: attributes to be set
- * @dp: timestamp delegation held by the SETATTR's sender
- * @deleg_attrs: FATTR4_WORD2_TIME_DELEG_* bits in the SETATTR
+ * nfsd4_vet_deleg_attrs - vet a timestamp delegation holder's times
+ * @iattr: holder's times in ia_atime and ia_mtime
+ * @dp: holder's timestamp delegation
+ * @deleg_attrs: FATTR4_WORD2_TIME_DELEG_* bits naming the times to vet
  *
- * Drop requested timestamps that don't pass nfsd4_vet_deleg_time(), and
- * clamp ones in the future. Set dl_setattr if both the mtime and ctime
- * pass.
+ * Set the ATTR_[AMC]TIME* bits in @iattr for the times that pass
+ * nfsd4_vet_deleg_time(), clamping ones in the future, and clear them for
+ * the ones that don't. Ctime follows an applied mtime, or is set to the
+ * current time if that mtime doesn't pass dl_ctime: DELEGRETURN won't
+ * stamp it, and the change attribute may derive from it.
  *
  * Context: Caller holds the inode_lock across this, applying @iattr, and
  * nfsd4_record_deleg_attrs(), so cb_getattr_update_times() can't move
@@ -10741,21 +10759,21 @@ void nfsd4_vet_deleg_attrs(struct iattr *iattr, struct nfs4_delegation *dp,
 
 	lockdep_assert_held_write(&inode->i_rwsem);
 
-	if ((deleg_attrs & FATTR4_WORD2_TIME_DELEG_ACCESS) &&
-	    !nfsd4_vet_deleg_time(&iattr->ia_atime, &dp->dl_atime, &now))
+	if (deleg_attrs & FATTR4_WORD2_TIME_DELEG_ACCESS) {
 		iattr->ia_valid &= ~(ATTR_ATIME | ATTR_ATIME_SET);
+		if (nfsd4_vet_deleg_time(&iattr->ia_atime, &dp->dl_atime, &now))
+			iattr->ia_valid |= ATTR_ATIME | ATTR_ATIME_SET;
+	}
 
 	if (deleg_attrs & 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))
-				dp->dl_setattr = true;
-			else
-				iattr->ia_valid &= ~(ATTR_CTIME | ATTR_CTIME_SET);
-		} else {
-			iattr->ia_valid &= ~(ATTR_CTIME | ATTR_CTIME_SET |
-					     ATTR_MTIME | ATTR_MTIME_SET);
-		}
+		iattr->ia_valid &= ~(ATTR_MTIME | ATTR_MTIME_SET |
+				     ATTR_CTIME | ATTR_CTIME_SET);
+		if (!nfsd4_vet_deleg_time(&iattr->ia_mtime, &dp->dl_mtime, &now))
+			return;
+		iattr->ia_valid |= ATTR_MTIME | ATTR_MTIME_SET | ATTR_CTIME;
+		iattr->ia_ctime = iattr->ia_mtime;
+		if (nfsd4_vet_deleg_time(&iattr->ia_ctime, &dp->dl_ctime, &now))
+			iattr->ia_valid |= ATTR_CTIME_SET;
 	}
 }
 
@@ -10778,9 +10796,12 @@ void nfsd4_record_deleg_attrs(const struct iattr *iattr,
 
 	if (iattr->ia_valid & ATTR_ATIME_SET)
 		dp->dl_atime = iattr->ia_atime;
-	if (iattr->ia_valid & ATTR_MTIME_SET)
+	if (iattr->ia_valid & ATTR_MTIME_SET) {
 		dp->dl_mtime = iattr->ia_mtime;
-	/* inode_set_ctime_deleg() keeps a later ctime */
+		/* Don't let DELEGRETURN stamp over the holder's mtime */
+		dp->dl_setattr = true;
+	}
+	/* Ctime may be "now", or inode_set_ctime_deleg() kept a later one */
 	if (iattr->ia_valid & ATTR_CTIME)
 		dp->dl_ctime = inode_get_ctime(inode);
 }
@@ -10810,8 +10831,6 @@ static void cb_getattr_update_times(const struct path *path,
 	/* Serialize vetting against dl_*time with their update below */
 	inode_lock(inode);
 	if (deleg_attrs_deleg(dp->dl_type)) {
-		struct timespec64 now = current_time(inode);
-
 		/*
 		 * The holder's own SETATTRs, xattr changes, fallocates, LINKs
 		 * etc. don't break its delegation and don't show up in its
@@ -10826,16 +10845,9 @@ static void cb_getattr_update_times(const struct path *path,
 
 		attrs.ia_atime = ncf->ncf_cb_atime;
 		attrs.ia_mtime = ncf->ncf_cb_mtime;
-
-		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, &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;
-		}
+		nfsd4_vet_deleg_attrs(&attrs, dp,
+				      FATTR4_WORD2_TIME_DELEG_ACCESS |
+				      FATTR4_WORD2_TIME_DELEG_MODIFY);
 
 		/*
 		 * The times are the holder's, so don't hold the GETATTR

-- 
2.56.0