Re: [PATCH v2] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated

[Sagi Grimberg <sagi@grimberg.me>]

Hey,

Chuck, this does not introduce any new xfstests failures.

Jeff, forgot to add your "Reviewed-by" tag, care to add another one?

Trond, from your last comment my understanding is that you are
supportive of this, are you planning to take this patch?

On 12/05/2025 0:54, Sagi Grimberg wrote:
> nfs_setattr will flush all pending writes before updating a file time
> attributes. However when the client holds delegated timestamps, it can
> update its timestamps locally as it is the authority for the file
> times attributes. The client will later set the file attributes by
> adding a setattr to the delegreturn compound updating the server time
> attributes.
>
> Fix nfs_setattr to avoid flushing pending writes when the file time
> attributes are delegated and the mtime/atime are set to a fixed
> timestamp (ATTR_[MODIFY|ACCESS]_SET. Also, when sending the setattr
> procedure over the wire, we need to clear the correct attribute bits
> from the bitmask.
>
> I was able to measure a noticable speedup when measuring untar performance.
> Test: $ time tar xzf ~/dir.tgz
> Baseline: 0m44.407s
> Patched: 0m29.712s
>
> Which is more than 30% latency improvement.
>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> Changes from v1:
> - Moved attr->ia_valid assignments as well as inode->lock handling to
>    nfs_set_timestamps_to_ts helper
>
>   fs/nfs/inode.c    | 54 +++++++++++++++++++++++++++++++++++++++++------
>   fs/nfs/nfs4proc.c |  8 +++----
>   2 files changed, 52 insertions(+), 10 deletions(-)
>
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 119e447758b9..295e2776053c 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -633,6 +633,40 @@ nfs_fattr_fixup_delegated(struct inode *inode, struct nfs_fattr *fattr)
>   	}
>   }
>   
> +static void nfs_set_timestamps_to_ts(struct inode *inode, struct iattr *attr)
> +{
> +	unsigned int cache_flags = 0;
> +
> +	spin_lock(&inode->i_lock);
> +	if (attr->ia_valid & ATTR_MTIME_SET) {
> +		struct timespec64 ctime = inode_get_ctime(inode);
> +		struct timespec64 mtime = inode_get_mtime(inode);
> +		struct timespec64 now;
> +		int updated = 0;
> +
> +		now = inode_set_ctime_current(inode);
> +		if (!timespec64_equal(&now, &ctime))
> +			updated |= S_CTIME;
> +
> +		inode_set_mtime_to_ts(inode, attr->ia_mtime);
> +		if (!timespec64_equal(&now, &mtime))
> +			updated |= S_MTIME;
> +
> +		inode_maybe_inc_iversion(inode, updated);
> +		cache_flags |= NFS_INO_INVALID_CTIME | NFS_INO_INVALID_MTIME;
> +		attr->ia_valid &= ~(ATTR_MTIME|ATTR_MTIME_SET|
> +					ATTR_ATIME|ATTR_ATIME_SET);
> +
> +	}
> +	if (attr->ia_valid & ATTR_ATIME_SET) {
> +		inode_set_atime_to_ts(inode, attr->ia_atime);
> +		cache_flags |= NFS_INO_INVALID_ATIME;
> +		attr->ia_valid &= ~(ATTR_ATIME|ATTR_ATIME_SET);
> +	}
> +	NFS_I(inode)->cache_validity &= ~cache_flags;
> +	spin_unlock(&inode->i_lock);
> +}
> +
>   static void nfs_update_timestamps(struct inode *inode, unsigned int ia_valid)
>   {
>   	enum file_time_flags time_flags = 0;
> @@ -700,15 +734,23 @@ nfs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>   	}
>   
>   	if (nfs_have_delegated_mtime(inode) && attr->ia_valid & ATTR_MTIME) {
> -		spin_lock(&inode->i_lock);
> -		nfs_update_timestamps(inode, attr->ia_valid);
> -		spin_unlock(&inode->i_lock);
> -		attr->ia_valid &= ~(ATTR_MTIME | ATTR_ATIME);
> +		if (attr->ia_valid & ATTR_MTIME_SET) {
> +			nfs_set_timestamps_to_ts(inode, attr);
> +		} else {
> +			spin_lock(&inode->i_lock);
> +			nfs_update_timestamps(inode, attr->ia_valid);
> +			spin_unlock(&inode->i_lock);
> +			attr->ia_valid &= ~(ATTR_MTIME|ATTR_ATIME);
> +		}
>   	} else if (nfs_have_delegated_atime(inode) &&
>   		   attr->ia_valid & ATTR_ATIME &&
>   		   !(attr->ia_valid & ATTR_MTIME)) {
> -		nfs_update_delegated_atime(inode);
> -		attr->ia_valid &= ~ATTR_ATIME;
> +		if (attr->ia_valid & ATTR_ATIME_SET) {
> +			nfs_set_timestamps_to_ts(inode, attr);
> +		} else {
> +			nfs_update_delegated_atime(inode);
> +			attr->ia_valid &= ~ATTR_ATIME;
> +		}
>   	}
>   
>   	/* Optimization: if the end result is no change, don't RPC */
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 970f28dbf253..c501a0d5da90 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -325,14 +325,14 @@ static void nfs4_bitmap_copy_adjust(__u32 *dst, const __u32 *src,
>   
>   	if (nfs_have_delegated_mtime(inode)) {
>   		if (!(cache_validity & NFS_INO_INVALID_ATIME))
> -			dst[1] &= ~FATTR4_WORD1_TIME_ACCESS;
> +			dst[1] &= ~(FATTR4_WORD1_TIME_ACCESS|FATTR4_WORD1_TIME_ACCESS_SET);
>   		if (!(cache_validity & NFS_INO_INVALID_MTIME))
> -			dst[1] &= ~FATTR4_WORD1_TIME_MODIFY;
> +			dst[1] &= ~(FATTR4_WORD1_TIME_MODIFY|FATTR4_WORD1_TIME_MODIFY_SET);
>   		if (!(cache_validity & NFS_INO_INVALID_CTIME))
> -			dst[1] &= ~FATTR4_WORD1_TIME_METADATA;
> +			dst[1] &= ~(FATTR4_WORD1_TIME_METADATA|FATTR4_WORD1_TIME_MODIFY_SET);
>   	} else if (nfs_have_delegated_atime(inode)) {
>   		if (!(cache_validity & NFS_INO_INVALID_ATIME))
> -			dst[1] &= ~FATTR4_WORD1_TIME_ACCESS;
> +			dst[1] &= ~(FATTR4_WORD1_TIME_ACCESS|FATTR4_WORD1_TIME_ACCESS_SET);
>   	}
>   }
>