* [PATCH v2] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated
@ 2025-05-11 21:54 Sagi Grimberg
2025-05-17 9:49 ` Sagi Grimberg
0 siblings, 1 reply; 2+ messages in thread
From: Sagi Grimberg @ 2025-05-11 21:54 UTC (permalink / raw)
To: linux-nfs; +Cc: Chuck Lever, Jeff Layton, Trond Myklebust, Anna Schumaker
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);
}
}
--
2.43.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated
2025-05-11 21:54 [PATCH v2] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated Sagi Grimberg
@ 2025-05-17 9:49 ` Sagi Grimberg
0 siblings, 0 replies; 2+ messages in thread
From: Sagi Grimberg @ 2025-05-17 9:49 UTC (permalink / raw)
To: linux-nfs; +Cc: Chuck Lever, Jeff Layton, Trond Myklebust, Anna Schumaker
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);
> }
> }
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-05-17 9:49 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-11 21:54 [PATCH v2] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated Sagi Grimberg
2025-05-17 9:49 ` Sagi Grimberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox