From: Olga Kornievskaia <aglo@umich.edu>
To: Jeff Layton <jlayton@kernel.org>
Cc: Trond Myklebust <trondmy@kernel.org>,
Anna Schumaker <anna@kernel.org>,
linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] nfs: fix utimensat() for atime with delegated timestamps
Date: Tue, 7 Apr 2026 17:24:38 -0400 [thread overview]
Message-ID: <CAN-5tyEgO2rk=nveNigFLroVGbExC=Ok3UYFGUrcBUGQ5f-i+w@mail.gmail.com> (raw)
In-Reply-To: <CAN-5tyF2nxMaGsQS1B3oAMizGoCGvnTBYNgS9kMOfNoKKcwHxA@mail.gmail.com>
On Tue, Apr 7, 2026 at 9:28 AM Olga Kornievskaia <aglo@umich.edu> 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@kernel.org> 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@umich.edu>
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > > 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@kernel.org>
next prev parent reply other threads:[~2026-04-07 21:24 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-24 17:32 [PATCH v2 0/2] nfs: delegated attribute fixes Jeff Layton
2026-03-24 17:32 ` [PATCH v2 1/2] nfs: fix utimensat() for atime with delegated timestamps Jeff Layton
2026-03-25 12:30 ` Jeff Layton
2026-04-07 13:28 ` Olga Kornievskaia
2026-04-07 21:24 ` Olga Kornievskaia [this message]
2026-03-24 17:32 ` [PATCH v2 2/2] nfs: update inode ctime after removexattr operation Jeff Layton
2026-03-27 15:11 ` Olga Kornievskaia
2026-03-27 15:50 ` Jeff Layton
2026-03-27 16:20 ` Olga Kornievskaia
2026-03-27 16:59 ` Jeff Layton
2026-03-27 18:22 ` Thomas Haynes
2026-03-27 19:36 ` Olga Kornievskaia
2026-03-27 20:02 ` Thomas Haynes
2026-03-31 18:17 ` Olga Kornievskaia
2026-03-31 18:50 ` Thomas Haynes
2026-03-31 21:10 ` Olga Kornievskaia
2026-03-31 22:57 ` Rick Macklem
2026-03-31 11:42 ` Jeff Layton
2026-03-31 18:47 ` Thomas Haynes
2026-03-31 21:19 ` Jeff Layton
2026-03-31 22:16 ` Thomas Haynes
2026-03-31 22:36 ` Jeff Layton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAN-5tyEgO2rk=nveNigFLroVGbExC=Ok3UYFGUrcBUGQ5f-i+w@mail.gmail.com' \
--to=aglo@umich.edu \
--cc=anna@kernel.org \
--cc=jlayton@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=trondmy@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).