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

[Chuck Lever <chuck.lever@oracle.com>]

On 4/25/25 8:49 AM, 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.

That's significant. Next step is to ensure this doesn't cause any
functional regressions. Have you run fstests ?


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


-- 
Chuck Lever