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