* Re: [PATCH] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated
2025-04-25 12:49 [PATCH] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated Sagi Grimberg
@ 2025-05-04 6:45 ` Sagi Grimberg
2025-05-05 13:23 ` Jeff Layton
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Sagi Grimberg @ 2025-05-04 6:45 UTC (permalink / raw)
To: linux-nfs
On 25/04/2025 15:49, 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.
Jeff, Trond, Anna,
Any comments on this patch?
>
> 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);
> }
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated
2025-04-25 12:49 [PATCH] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated Sagi Grimberg
2025-05-04 6:45 ` Sagi Grimberg
@ 2025-05-05 13:23 ` Jeff Layton
2025-05-06 13:43 ` Sagi Grimberg
2025-05-05 13:25 ` Chuck Lever
2025-05-06 15:58 ` Trond Myklebust
3 siblings, 1 reply; 10+ messages in thread
From: Jeff Layton @ 2025-05-05 13:23 UTC (permalink / raw)
To: sagi, linux-nfs; +Cc: Trond Myklebust, Anna Schumaker, Thomas Haynes
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>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated
2025-05-05 13:23 ` Jeff Layton
@ 2025-05-06 13:43 ` Sagi Grimberg
2025-05-06 13:52 ` Jeff Layton
0 siblings, 1 reply; 10+ messages in thread
From: Sagi Grimberg @ 2025-05-06 13:43 UTC (permalink / raw)
To: Jeff Layton, linux-nfs; +Cc: Trond Myklebust, Anna Schumaker, Thomas Haynes
Hey Jeff,
On 05/05/2025 16:23, Jeff Layton wrote:
> 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!
Indeed,
Do note that in order to get this change, I made nfsd send a space_limit
(see under the
'---' separator) such that file close will not flush buffered data if it
amounts to less than
the space_limit. Without this small patch, the flush invoked from close
synchronizes everything
and making setattr async does not buy us as much.
>
>> 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?
I don't think so, the delegation holder is allowed to set attributes
against the
server if it so chooses.
>
>> + 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().
I tried to stay consistent with the nfs_update_timestamps() call. But I can
move it. sure.
>
>
>> + } 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.
Can you elaborate? didn't notice that git uses the times ATTR_*_SET
variant too often.
>
> Otherwise, this seems like a reasonable thing to do, and I do like the
> potential performance improvement!
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated
2025-05-06 13:43 ` Sagi Grimberg
@ 2025-05-06 13:52 ` Jeff Layton
2025-05-06 14:25 ` Sagi Grimberg
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Layton @ 2025-05-06 13:52 UTC (permalink / raw)
To: Sagi Grimberg, linux-nfs; +Cc: Trond Myklebust, Anna Schumaker, Thomas Haynes
On Tue, 2025-05-06 at 16:43 +0300, Sagi Grimberg wrote:
> Hey Jeff,
>
> On 05/05/2025 16:23, Jeff Layton wrote:
> > 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!
>
> Indeed,
>
> Do note that in order to get this change, I made nfsd send a space_limit
> (see under the
> '---' separator) such that file close will not flush buffered data if it
> amounts to less than
> the space_limit. Without this small patch, the flush invoked from close
> synchronizes everything
> and making setattr async does not buy us as much.
>
> >
> > > 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?
>
> I don't think so, the delegation holder is allowed to set attributes
> against the
> server if it so chooses.
>
> >
> > > + 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().
>
> I tried to stay consistent with the nfs_update_timestamps() call. But I can
> move it. sure.
>
> >
> >
> > > + } 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.
>
> Can you elaborate? didn't notice that git uses the times ATTR_*_SET
> variant too often.
>
Unfortunately, not much.
If you turn on attribute delegation, and then run the git regression
suite in a highly-threaded configuration, some of the tests fail. I've
made a couple of stabs at trying to narrow down a the reproducer, but
no luck so far.
My guess is that it's a client-side bug:
The server is fairly simple here -- if there is an outstanding
delegation, it asks the delegation holder for attributes via CB_GETATTR
and then passes those along to the client.
The client however has traditionally relied on the server to provide
updated attributes, rather than handling timestamps itself, and it
wouldn't surprise me if it just didn't get all of those places right.
FWIW, I did try your patch with that test and it didn't help.
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated
2025-05-06 13:52 ` Jeff Layton
@ 2025-05-06 14:25 ` Sagi Grimberg
2025-05-06 14:29 ` Jeff Layton
0 siblings, 1 reply; 10+ messages in thread
From: Sagi Grimberg @ 2025-05-06 14:25 UTC (permalink / raw)
To: Jeff Layton, linux-nfs; +Cc: Trond Myklebust, Anna Schumaker, Thomas Haynes
>>>>
>>> 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.
>> Can you elaborate? didn't notice that git uses the times ATTR_*_SET
>> variant too often.
>>
> Unfortunately, not much.
>
> If you turn on attribute delegation, and then run the git regression
> suite in a highly-threaded configuration, some of the tests fail. I've
> made a couple of stabs at trying to narrow down a the reproducer, but
> no luck so far.
>
> My guess is that it's a client-side bug:
>
> The server is fairly simple here -- if there is an outstanding
> delegation, it asks the delegation holder for attributes via CB_GETATTR
> and then passes those along to the client.
>
> The client however has traditionally relied on the server to provide
> updated attributes, rather than handling timestamps itself, and it
> wouldn't surprise me if it just didn't get all of those places right.
It's running, not failing yet... Which test cases fail?
>
> FWIW, I did try your patch with that test and it didn't help.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated
2025-05-06 14:25 ` Sagi Grimberg
@ 2025-05-06 14:29 ` Jeff Layton
0 siblings, 0 replies; 10+ messages in thread
From: Jeff Layton @ 2025-05-06 14:29 UTC (permalink / raw)
To: Sagi Grimberg, linux-nfs; +Cc: Trond Myklebust, Anna Schumaker, Thomas Haynes
On Tue, 2025-05-06 at 17:25 +0300, Sagi Grimberg wrote:
> > > > >
> > > > >
> > > > >
> > > > >
> > > > 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.
> > > Can you elaborate? didn't notice that git uses the times ATTR_*_SET
> > > variant too often.
> > >
> > Unfortunately, not much.
> >
> > If you turn on attribute delegation, and then run the git regression
> > suite in a highly-threaded configuration, some of the tests fail. I've
> > made a couple of stabs at trying to narrow down a the reproducer, but
> > no luck so far.
> >
> > My guess is that it's a client-side bug:
> >
> > The server is fairly simple here -- if there is an outstanding
> > delegation, it asks the delegation holder for attributes via CB_GETATTR
> > and then passes those along to the client.
> >
> > The client however has traditionally relied on the server to provide
> > updated attributes, rather than handling timestamps itself, and it
> > wouldn't surprise me if it just didn't get all of those places right.
>
> It's running, not failing yet... Which test cases fail?
I typically run this under kdevops, using the "stress" profile (which
runs 2 threads for every CPU in the system):
jlayton@walgis:~/git/kdevops/workflows/gitr/results/6.15.0-rc5-ga182e4cc7f89/nfs-v42$ cat 20250505T100749.summary
Test Summary Report
-------------------
./t4137-apply-submodule.sh (Wstat: 256 (exited 1) Tests: 28 Failed: 1)
Failed test: 22
Non-zero exit status: 1
./t5701-git-serve.sh (Wstat: 256 (exited 1) Tests: 24 Failed: 2)
Failed tests: 1, 21
Non-zero exit status: 1
./t7112-reset-submodule.sh (Wstat: 256 (exited 1) Tests: 82 Failed: 3)
Failed tests: 6-7, 20
Non-zero exit status: 1
./t1092-sparse-checkout-compatibility.sh (Wstat: 256 (exited 1) Tests: 98 Failed: 20)
Failed tests: 9, 12, 14, 21, 27, 31-39, 48, 50-54
Non-zero exit status: 1
Files=1007, Tests=30920, 12452 wallclock secs (13.38 usr 12.64 sys + 1115.57 cusr 16188.83 csys = 17330.42 CPU)
Result: FAIL
My efforts at narrowing down the testsuite have mostly resulted in
things not failing. It seems to be some combination of the tests, and
the fact that they are all running in parallel.
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated
2025-04-25 12:49 [PATCH] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated Sagi Grimberg
2025-05-04 6:45 ` Sagi Grimberg
2025-05-05 13:23 ` Jeff Layton
@ 2025-05-05 13:25 ` Chuck Lever
2025-05-06 13:45 ` Sagi Grimberg
2025-05-06 15:58 ` Trond Myklebust
3 siblings, 1 reply; 10+ messages in thread
From: Chuck Lever @ 2025-05-05 13:25 UTC (permalink / raw)
To: sagi, linux-nfs; +Cc: Trond Myklebust, Jeff Layton, Anna Schumaker
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated
2025-05-05 13:25 ` Chuck Lever
@ 2025-05-06 13:45 ` Sagi Grimberg
0 siblings, 0 replies; 10+ messages in thread
From: Sagi Grimberg @ 2025-05-06 13:45 UTC (permalink / raw)
To: Chuck Lever, linux-nfs; +Cc: Trond Myklebust, Jeff Layton, Anna Schumaker
On 05/05/2025 16:25, Chuck Lever wrote:
> 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 ?
I have not. But I will. Please note the comment under the '---' separator.
This only make a difference if nfsd grants the client a space_limit with
the write deleg. Without it, untar is still very much synchronous.
>> 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated
2025-04-25 12:49 [PATCH] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated Sagi Grimberg
` (2 preceding siblings ...)
2025-05-05 13:25 ` Chuck Lever
@ 2025-05-06 15:58 ` Trond Myklebust
3 siblings, 0 replies; 10+ messages in thread
From: Trond Myklebust @ 2025-05-06 15:58 UTC (permalink / raw)
To: linux-nfs@vger.kernel.org, sagi@grimberg.me
Cc: jlayton@kernel.org, Anna.Schumaker@netapp.com
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.
Yes, I think this is a good change. I can't remember why we chose not
to do this back when we developed the delegated timestamps, but I
suspect it was just lack of maturity of the protocol at the time.
I certainly don't see any obvious reason why it would be wrong.
>
> 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_ATIM
> E_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);
> }
> }
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply [flat|nested] 10+ messages in thread