Re: [PATCH v2 1/2] nfs: fix utimensat() for atime with delegated timestamps

[Olga Kornievskaia]

On Tue, Apr 7, 2026 at 9:28 AM Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
>
> Hi Jeff and all,
>
> With this patch and running against a server that would update the
> c/mtime of a GETATTR in a compound with CLONE(COPY),  the timestamps
> is (might be) going "backwards"? I see that SETATTR+DELEGRETURN (after
> the CLONE) is setting a timestamp which is earlier than what's
> returned by the server in the GETATTR. Though I haven't figured out
> how to "show" it via test (I observe it on a network trace).
>
> This is seen testing either against Hammerspace which already updates
> the values or the linux server with the patch I'm working on to update
> timestamp for CLONE/COPY.
>
> I have to say I haven't figured out if this patch needs to modify such
> that it fixes 221 but doesn't break 407 or we are fundamentally
> dealing with a problem of including a GETATTR in a compound while
> holding an attribute delegation that needs to be solved differently.
> Like I said before, if the server were to update the value upon
> receiving the GETATTR in the compound (regardless of the origin),
> should it then do a CB_GETATTR first (which I already grumbled seems
> like a poor choice)? Or, should the client be changed to not send a
> GETATTR in CLONE of the compounds if it's holding an attribute
> delegation and somehow figure out attributes differently then it does
> now.

Apologies. Please excuse the noise about the mtime going backwards.
There are 2 SETATTRs going on and I was looking at the src file
SETATTR. I believe I was also confused by the difference between
time_access value from time_modify in the SETATTR (for the dest file).
It's unclear why the access time wasn't modified (and the value
returned by the OPEN's GETATTR is used) but the mtime was modified
based on what's received in the CLONE's GETATTR.

> On Wed, Mar 25, 2026 at 8:30 AM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >
> > On Tue, 2026-03-24 at 13:32 -0400, Jeff Layton wrote:
> > > xfstest generic/221 is failing with delegated timestamps enabled.  When
> > > the client holds a WRITE_ATTRS_DELEG delegation, and a userland process
> > > does a utimensat() for only the atime, the ctime is not properly
> > > updated. The problem is that the client tries to cache the atime update,
> > > but there is no mtime update, so the delegated attribute update never
> > > updates the ctime.
> > >
> > > Delegated timestamps don't have a mechanism to update the ctime in
> > > accordance with atime-only changes due to utimensat() and the like.
> > > Change the client to issue an RPC in this case, so that the ctime gets
> > > properly updated alongside the atime.
> > >
> > > Fixes: 40f45ab3814f ("NFS: Further fixes to attribute delegation a/mtime changes")
> > > Reported-by: Olga Kornievskaia <aglo@xxxxxxxxx>
> > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > ---
> > >  fs/nfs/inode.c | 9 +--------
> > >  1 file changed, 1 insertion(+), 8 deletions(-)
> > >
> > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > > index 4786343eeee0f874aa1f31ace2f35fdcb83fc7a6..3a5bba7e3c92d4d4fcd65234cd2f10e56f78dee0 100644
> > > --- a/fs/nfs/inode.c
> > > +++ b/fs/nfs/inode.c
> > > @@ -757,14 +757,7 @@ nfs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> > >       } else if (nfs_have_delegated_atime(inode) &&
> > >                  attr->ia_valid & ATTR_ATIME &&
> > >                  !(attr->ia_valid & ATTR_MTIME)) {
> > > -             if (attr->ia_valid & ATTR_ATIME_SET) {
> > > -                     if (uid_eq(task_uid, owner_uid)) {
> > > -                             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 {
> >
> > This probably deserves a comment. How about something like:
> >
> >                 /*
> >                  * An atime-only update via an explicit setattr (e.g.: utimensat() and
> >                  * the like) requires updating the ctime as well. Delegated timestamps
> >                  * don't have a mechanism for updating the ctime with a delegated
> >                  * atime-only update, so an RPC must be issued.
> >                  */
> >
> > > +             if (!(attr->ia_valid & ATTR_ATIME_SET)) {
> > >                       nfs_update_delegated_atime(inode);
> > >                       attr->ia_valid &= ~ATTR_ATIME;
> > >               }
> >
> > --
> > Jeff Layton <jlayton@xxxxxxxxxx>