On 7/26/25 3:03 PM, Jeff Layton wrote:

On Sat, 2025-07-26 at 14:48 -0400, Chuck Lever wrote:

quoted

Hi Jeff -

Thanks again for your focus on getting this straightened out!


On 7/26/25 10:31 AM, Jeff Layton wrote:

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

quoted

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