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

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2025-04-25 at 15:49 +0300, 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: 1m13.072s
> Patched: 0m49.038s
> 
> Which is more than 30% latency improvement.
> 

(cc'ing Tom since he was the spec author for the timestamp delegation)

Nice!

> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> Tested this on a vm in my laptop against chuck nfsd-testing which
> grants write delegs for write-only opens, plus another small modparam
> that also adds a space_limit to the delegation.
> 
>  fs/nfs/inode.c    | 49 +++++++++++++++++++++++++++++++++++++++++++----
>  fs/nfs/nfs4proc.c |  8 ++++----
>  2 files changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 119e447758b9..6472b95bfd88 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -633,6 +633,34 @@ 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;
> +
> +	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;
> +	}
> +	if (attr->ia_valid & ATTR_ATIME_SET) {
> +		inode_set_atime_to_ts(inode, attr->ia_atime);
> +		cache_flags |= NFS_INO_INVALID_ATIME;
> +	}
> +	NFS_I(inode)->cache_validity &= ~cache_flags;
> +}
> +
>  static void nfs_update_timestamps(struct inode *inode, unsigned int ia_valid)
>  {
>  	enum file_time_flags time_flags = 0;
> @@ -701,14 +729,27 @@ 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);
> +		if (attr->ia_valid & ATTR_MTIME_SET) {

Is this also a bugfix? Is ATTR_MTIME_SET being handled correctly in the
existing code?

> +			nfs_set_timestamps_to_ts(inode, attr);
> +			attr->ia_valid &= ~(ATTR_MTIME|ATTR_MTIME_SET|
> +						ATTR_ATIME|ATTR_ATIME_SET);

It might look a little cleaner to move the ia_valid changes into
nfs_set_timestamps_to_ts().


> +		} else {
> +			nfs_update_timestamps(inode, attr->ia_valid);
> +			attr->ia_valid &= ~(ATTR_MTIME|ATTR_ATIME);
> +		}
>  		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) {
> +			spin_lock(&inode->i_lock);
> +			nfs_set_timestamps_to_ts(inode, attr);
> +			spin_unlock(&inode->i_lock);
> +			attr->ia_valid &= ~(ATTR_ATIME|ATTR_ATIME_SET);
> +		} 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);
>  	}
>  }
>  

FWIW, we've been chasing some problems with the git regression
testsuite when attribute delegation is enabled. It would be interesting
to test this patch to see if it changes that behavior.

Otherwise, this seems like a reasonable thing to do, and I do like the
potential performance improvement!

Reviewed-by: Jeff Layton <jlayton@kernel.org>