[PATCH 0/2] vfs: fix delegated timestamp updates

[PATCH 0/2] vfs: fix delegated timestamp updates

[Jeff Layton]

We had been seeing some failures in the git regression testsuite when
run on NFS with delegated timestamps enabled. The first patch adds a
tracepoint that was helpful for tracking down the problem. The second
patch _mostly_ fixes the actual issue. With this, the git regression
testsuite is passing a lot more often for me, even when run in the
"stress" configuration under kdevops.

That said, I'm still seeing an occasional failure that I think may be a
problem on the client. I'll send email about that separately.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Jeff Layton (2):
      vfs: add tracepoints in inode_set_ctime_deleg
      vfs: fix delegated timestamp handling in setattr_copy()

 fs/attr.c                        | 52 +++++++++++++++++++++++++++++-----------
 fs/inode.c                       |  5 +++-
 fs/nfsd/nfs4xdr.c                |  4 +---
 include/trace/events/timestamp.h | 40 +++++++++++++++++++++++++++++++
 4 files changed, 83 insertions(+), 18 deletions(-)
---
base-commit: bab771b8eba6f3b13446ced52751be122af0d3b7
change-id: 20250722-nfsd-testing-5e861a3cf3a0

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

[PATCH 1/2] vfs: add tracepoints in inode_set_ctime_deleg

[Jeff Layton]

Add tracepoints in inode_set_ctime_deleg() that show the existing ctime,
the requested ctime and the current_time().

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/inode.c                       |  5 ++++-
 include/trace/events/timestamp.h | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/fs/inode.c b/fs/inode.c
index 99318b157a9a13b3dd8dad0f5f90951f08ef64de..6a8bf57d649aa0909b85f09e3b5b0fbc81efe303 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2811,10 +2811,13 @@ struct timespec64 inode_set_ctime_deleg(struct inode *inode, struct timespec64 u
 	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)
+	if (timespec64_compare(&update, &cur_ts) <= 0) {
+		trace_inode_set_ctime_deleg(inode, &cur_ts, &update, NULL);
 		return cur_ts;
+	}
 
 	ktime_get_coarse_real_ts64_mg(&now);
+	trace_inode_set_ctime_deleg(inode, &cur_ts, &update, &now);
 
 	/* Clamp the update to "now" if it's in the future */
 	if (timespec64_compare(&update, &now) > 0)
diff --git a/include/trace/events/timestamp.h b/include/trace/events/timestamp.h
index c9e5ec930054887a6a7bae8e487611b5ded33d71..e66161d8e14d9b74b0c875f0b324d24895403c18 100644
--- a/include/trace/events/timestamp.h
+++ b/include/trace/events/timestamp.h
@@ -118,6 +118,46 @@ TRACE_EVENT(fill_mg_cmtime,
 		__entry->mtime_s, __entry->mtime_ns
 	)
 );
+
+TRACE_EVENT(inode_set_ctime_deleg,
+	TP_PROTO(struct inode *inode,
+		 struct timespec64 *old,
+		 struct timespec64 *req,
+		 struct timespec64 *now),
+
+	TP_ARGS(inode, old, req, now),
+
+	TP_STRUCT__entry(
+		__field(dev_t,		dev)
+		__field(ino_t,		ino)
+		__field(time64_t,	old_s)
+		__field(time64_t,	req_s)
+		__field(time64_t,	now_s)
+		__field(u32,		old_ns)
+		__field(u32,		req_ns)
+		__field(u32,		now_ns)
+		__field(u32,		gen)
+	),
+
+	TP_fast_assign(
+		__entry->dev		= inode->i_sb->s_dev;
+		__entry->ino		= inode->i_ino;
+		__entry->gen		= inode->i_generation;
+		__entry->old_s		= old->tv_sec;
+		__entry->req_s		= req->tv_sec;
+		__entry->now_s		= now ? now->tv_sec : 0;
+		__entry->old_ns		= old->tv_nsec;
+		__entry->req_ns		= req->tv_nsec;
+		__entry->now_ns		= now ? now->tv_nsec : 0;
+	),
+
+	TP_printk("ino=%d:%d:%ld:%u old=%lld.%u req=%lld.%u now=%lld.%u",
+		MAJOR(__entry->dev), MINOR(__entry->dev), __entry->ino, __entry->gen,
+		__entry->old_s, __entry->old_ns,
+		__entry->req_s, __entry->req_ns,
+		__entry->now_s, __entry->now_ns
+	)
+);
 #endif /* _TRACE_TIMESTAMP_H */
 
 /* This part must be outside protection */

-- 
2.50.1

[PATCH 2/2] vfs: fix delegated timestamp handling in setattr_copy()

[Jeff Layton]

There are a couple of problems with delegated timestamp updates via
setattr:

1/ the ia_ctime is always clobbered by notify_change(), so setting the
ia_ctime to the same value as the ia_mtime in nfsd4_decode_fattr4()
doesn't work.

2/ while it does test the ctime's validity vs. the existing ctime and
current_time(), the same is not done for the atime or mtime. The spec
requires this.

Add a new setattr_copy_delegts() function that handles updating the
timestamps whenever ATTR_DELEG is set. For both atime and mtime,
validate and clamp the value to current_time(), and then set it. If the
mtime gets updated, also update the ctime.

Fixes: 7f2c86cba3c5 ("fs: handle delegated timestamps in setattr_copy_mgtime")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/attr.c         | 52 ++++++++++++++++++++++++++++++++++++++--------------
 fs/nfsd/nfs4xdr.c |  4 +---
 2 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index 9caf63d20d03e86c535e9c8c91d49c2a34d34b7a..3e636943d26a36aeeed0ff8b428b6dd3e63f8dde 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);
@@ -312,6 +305,39 @@ static void setattr_copy_mgtime(struct inode *inode, const struct iattr *attr)
 		inode_set_mtime_to_ts(inode, now);
 }
 
+/*
+ * Skip update if new value is older than existing time. Clamp
+ * to current_time() if it's in the future.
+ */
+static void setattr_copy_delegts(struct inode *inode, const struct iattr *attr)
+{
+	struct timespec64 now = current_time(inode);
+	unsigned int ia_valid = attr->ia_valid;
+
+	if (ia_valid & ATTR_MTIME) {
+		struct timespec64 cur = inode_get_mtime(inode);
+
+		if (timespec64_compare(&attr->ia_mtime, &cur) > 0) {
+			if (timespec64_compare(&attr->ia_mtime, &now) > 0)
+				inode_set_mtime_to_ts(inode, now);
+			else
+				inode_set_mtime_to_ts(inode, attr->ia_mtime);
+			inode_set_ctime_deleg(inode, attr->ia_mtime);
+		}
+	}
+
+	if (ia_valid & ATTR_ATIME) {
+		struct timespec64 cur = inode_get_atime(inode);
+
+		if (timespec64_compare(&attr->ia_atime, &cur) > 0) {
+			if (timespec64_compare(&attr->ia_atime, &now) > 0)
+				inode_set_atime_to_ts(inode, now);
+			else
+				inode_set_atime_to_ts(inode, attr->ia_atime);
+		}
+	}
+}
+
 /**
  * setattr_copy - copy simple metadata updates into the generic inode
  * @idmap:	idmap of the mount the inode was found from
@@ -352,6 +378,8 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode *inode,
 		inode->i_mode = mode;
 	}
 
+	if (ia_valid & ATTR_DELEG)
+		return setattr_copy_delegts(inode, attr);
 	if (is_mgtime(inode))
 		return setattr_copy_mgtime(inode, attr);
 
@@ -359,12 +387,8 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode *inode,
 		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/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 8b68f74a8cf08c6aa1305a2a3093467656085e4a..e6899a3502332d686138abee2284c87fc7fbc0ae 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -537,9 +537,7 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
 			return nfserr_bad_xdr;
 		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_valid |= ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
 	}
 
 	/* request sanity: did attrlist4 contain the expected number of words? */

-- 
2.50.1

Re: [PATCH 2/2] vfs: fix delegated timestamp handling in setattr_copy()

[Jeff Layton]

On Tue, 2025-07-22 at 14:52 -0400, Jeff Layton wrote:
> There are a couple of problems with delegated timestamp updates via
> setattr:
> 
> 1/ the ia_ctime is always clobbered by notify_change(), so setting the
> ia_ctime to the same value as the ia_mtime in nfsd4_decode_fattr4()
> doesn't work.
> 
> 2/ while it does test the ctime's validity vs. the existing ctime and
> current_time(), the same is not done for the atime or mtime. The spec
> requires this.
> 
> Add a new setattr_copy_delegts() function that handles updating the
> timestamps whenever ATTR_DELEG is set. For both atime and mtime,
> validate and clamp the value to current_time(), and then set it. If the
> mtime gets updated, also update the ctime.
> 
> Fixes: 7f2c86cba3c5 ("fs: handle delegated timestamps in setattr_copy_mgtime")
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/attr.c         | 52 ++++++++++++++++++++++++++++++++++++++--------------
>  fs/nfsd/nfs4xdr.c |  4 +---
>  2 files changed, 39 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/attr.c b/fs/attr.c
> index 9caf63d20d03e86c535e9c8c91d49c2a34d34b7a..3e636943d26a36aeeed0ff8b428b6dd3e63f8dde 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);
> @@ -312,6 +305,39 @@ static void setattr_copy_mgtime(struct inode *inode, const struct iattr *attr)
>  		inode_set_mtime_to_ts(inode, now);
>  }
>  
> +/*
> + * Skip update if new value is older than existing time. Clamp
> + * to current_time() if it's in the future.
> + */
> +static void setattr_copy_delegts(struct inode *inode, const struct iattr *attr)
> +{
> +	struct timespec64 now = current_time(inode);
> +	unsigned int ia_valid = attr->ia_valid;
> +
> +	if (ia_valid & ATTR_MTIME) {
> +		struct timespec64 cur = inode_get_mtime(inode);
> +
> +		if (timespec64_compare(&attr->ia_mtime, &cur) > 0) {
> +			if (timespec64_compare(&attr->ia_mtime, &now) > 0)
> +				inode_set_mtime_to_ts(inode, now);
> +			else
> +				inode_set_mtime_to_ts(inode, attr->ia_mtime);
> +			inode_set_ctime_deleg(inode, attr->ia_mtime);
> +		}
> +	}
> +
> +	if (ia_valid & ATTR_ATIME) {
> +		struct timespec64 cur = inode_get_atime(inode);
> +
> +		if (timespec64_compare(&attr->ia_atime, &cur) > 0) {
> +			if (timespec64_compare(&attr->ia_atime, &now) > 0)
> +				inode_set_atime_to_ts(inode, now);
> +			else
> +				inode_set_atime_to_ts(inode, attr->ia_atime);
> +		}
> +	}
> +}
> +
>  /**
>   * setattr_copy - copy simple metadata updates into the generic inode
>   * @idmap:	idmap of the mount the inode was found from
> @@ -352,6 +378,8 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode *inode,
>  		inode->i_mode = mode;
>  	}
>  
> +	if (ia_valid & ATTR_DELEG)
> +		return setattr_copy_delegts(inode, attr);
>  	if (is_mgtime(inode))
>  		return setattr_copy_mgtime(inode, attr);
>  
> @@ -359,12 +387,8 @@ void setattr_copy(struct mnt_idmap *idmap, struct inode *inode,
>  		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/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 8b68f74a8cf08c6aa1305a2a3093467656085e4a..e6899a3502332d686138abee2284c87fc7fbc0ae 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -537,9 +537,7 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
>  			return nfserr_bad_xdr;
>  		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_valid |= ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
>  	}
>  
>  	/* request sanity: did attrlist4 contain the expected number of words? */

Based on Trond's comments in this thread, I think I'm going to have to
respin this:

https://lore.kernel.org/linux-nfs/bfa20f4a81e0c2d5df8525476fb29af156f4f5f1.camel@kernel.org/

I'll post a v2 in the near future.

Cheers,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/2] vfs: add tracepoints in inode_set_ctime_deleg

[Steven Rostedt]

On Tue, 22 Jul 2025 14:52:27 -0400
Jeff Layton <jlayton@kernel.org> wrote:

> +	TP_fast_assign(
> +		__entry->dev		= inode->i_sb->s_dev;
> +		__entry->ino		= inode->i_ino;
> +		__entry->gen		= inode->i_generation;
> +		__entry->old_s		= old->tv_sec;
> +		__entry->req_s		= req->tv_sec;
> +		__entry->now_s		= now ? now->tv_sec : 0;
> +		__entry->old_ns		= old->tv_nsec;
> +		__entry->req_ns		= req->tv_nsec;
> +		__entry->now_ns		= now ? now->tv_nsec : 0;
> +	),
> +
> +	TP_printk("ino=%d:%d:%ld:%u old=%lld.%u req=%lld.%u now=%lld.%u",

Hmm, wouldn't you want the above to be:

	TP_printk("ino=%d:%d:%ld:%u old=%lld.%09u req=%lld.%09u now=%lld.%09u",

Otherwise the nanosecond part is going to look confusing if it's less that 100,000,000.

-- Steve


> +		MAJOR(__entry->dev), MINOR(__entry->dev), __entry->ino, __entry->gen,
> +		__entry->old_s, __entry->old_ns,
> +		__entry->req_s, __entry->req_ns,
> +		__entry->now_s, __entry->now_ns
> +	)

Re: [PATCH 2/2] vfs: fix delegated timestamp handling in setattr_copy()

[Christian Brauner]

> Based on Trond's comments in this thread, I think I'm going to have to
> respin this:
> 
> https://lore.kernel.org/linux-nfs/bfa20f4a81e0c2d5df8525476fb29af156f4f5f1.camel@kernel.org/
> 
> I'll post a v2 in the near future.

It's v6.8 material anyway so no rush. :)

Re: [PATCH 1/2] vfs: add tracepoints in inode_set_ctime_deleg

[Chuck Lever]

On 7/22/25 2:52 PM, Jeff Layton wrote:
> Add tracepoints in inode_set_ctime_deleg() that show the existing ctime,
> the requested ctime and the current_time().
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/inode.c                       |  5 ++++-
>  include/trace/events/timestamp.h | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 99318b157a9a13b3dd8dad0f5f90951f08ef64de..6a8bf57d649aa0909b85f09e3b5b0fbc81efe303 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2811,10 +2811,13 @@ struct timespec64 inode_set_ctime_deleg(struct inode *inode, struct timespec64 u
>  	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)
> +	if (timespec64_compare(&update, &cur_ts) <= 0) {
> +		trace_inode_set_ctime_deleg(inode, &cur_ts, &update, NULL);
>  		return cur_ts;
> +	}
>  
>  	ktime_get_coarse_real_ts64_mg(&now);
> +	trace_inode_set_ctime_deleg(inode, &cur_ts, &update, &now);
>  
>  	/* Clamp the update to "now" if it's in the future */
>  	if (timespec64_compare(&update, &now) > 0)
> diff --git a/include/trace/events/timestamp.h b/include/trace/events/timestamp.h
> index c9e5ec930054887a6a7bae8e487611b5ded33d71..e66161d8e14d9b74b0c875f0b324d24895403c18 100644
> --- a/include/trace/events/timestamp.h
> +++ b/include/trace/events/timestamp.h
> @@ -118,6 +118,46 @@ TRACE_EVENT(fill_mg_cmtime,
>  		__entry->mtime_s, __entry->mtime_ns
>  	)
>  );
> +
> +TRACE_EVENT(inode_set_ctime_deleg,
> +	TP_PROTO(struct inode *inode,
> +		 struct timespec64 *old,
> +		 struct timespec64 *req,
> +		 struct timespec64 *now),
> +
> +	TP_ARGS(inode, old, req, now),
> +
> +	TP_STRUCT__entry(
> +		__field(dev_t,		dev)
> +		__field(ino_t,		ino)
> +		__field(time64_t,	old_s)
> +		__field(time64_t,	req_s)
> +		__field(time64_t,	now_s)
> +		__field(u32,		old_ns)
> +		__field(u32,		req_ns)
> +		__field(u32,		now_ns)
> +		__field(u32,		gen)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->dev		= inode->i_sb->s_dev;
> +		__entry->ino		= inode->i_ino;
> +		__entry->gen		= inode->i_generation;
> +		__entry->old_s		= old->tv_sec;
> +		__entry->req_s		= req->tv_sec;
> +		__entry->now_s		= now ? now->tv_sec : 0;
> +		__entry->old_ns		= old->tv_nsec;
> +		__entry->req_ns		= req->tv_nsec;
> +		__entry->now_ns		= now ? now->tv_nsec : 0;
> +	),
> +
> +	TP_printk("ino=%d:%d:%ld:%u old=%lld.%u req=%lld.%u now=%lld.%u",
> +		MAJOR(__entry->dev), MINOR(__entry->dev), __entry->ino, __entry->gen,
> +		__entry->old_s, __entry->old_ns,
> +		__entry->req_s, __entry->req_ns,
> +		__entry->now_s, __entry->now_ns
> +	)
> +);
>  #endif /* _TRACE_TIMESTAMP_H */
>  
>  /* This part must be outside protection */
> 

Reviewed-by: Chuck Lever <chuck.lever@oracle.com>


-- 
Chuck Lever