Re: [PATCH v2 2/2] nfs: update inode ctime after removexattr operation

[Thomas Haynes <loghyr@gmail.com>]

On Tue, Mar 31, 2026 at 02:17:54PM -0800, Olga Kornievskaia wrote:
> On Fri, Mar 27, 2026 at 4:02 PM Thomas Haynes <loghyr@gmail.com> wrote:
> >
> > On Fri, Mar 27, 2026 at 03:36:12PM -0800, Olga Kornievskaia wrote:
> > > On Fri, Mar 27, 2026 at 2:22 PM Thomas Haynes <loghyr@gmail.com> wrote:
> > > >
> > > > On Fri, Mar 27, 2026 at 12:59:54PM -0800, Jeff Layton wrote:
> > > > > On Fri, 2026-03-27 at 12:20 -0400, Olga Kornievskaia wrote:
> > > > > > On Fri, Mar 27, 2026 at 11:50 AM Jeff Layton <jlayton@kernel.org> wrote:
> > > > > > >
> > > > > > > On Fri, 2026-03-27 at 11:11 -0400, Olga Kornievskaia wrote:
> > > > > > > > On Tue, Mar 24, 2026 at 1:32 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > > > > > >
> > > > > > > > > xfstest generic/728 fails with delegated timestamps. The client does a
> > > > > > > > > removexattr and then a stat to test the ctime, which doesn't change. The
> > > > > > > > > stat() doesn't trigger a GETATTR because of the delegated timestamps, so
> > > > > > > > > it relies on the cached ctime, which is wrong.
> > > > > > > > >
> > > > > > > > > The setxattr compound has a trailing GETATTR, which ensures that its
> > > > > > > > > ctime gets updated. Follow the same strategy with removexattr.
> > > > > > > >
> > > > > > > > This approach relies on the fact that the server the serves delegated
> > > > > > > > attributes would update change_attr on operations which might now
> > > > > > > > necessarily happen (ie, linux server does not update change_attribute
> > > > > > > > on writes or clone). I propose an alternative fix for the failing
> > > > > > > > generic/728.
> > > > > > > >
> > > > > > > > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> > > > > > > > index 7b3ca68fb4bb..ede1835a45b3 100644
> > > > > > > > --- a/fs/nfs/nfs42proc.c
> > > > > > > > +++ b/fs/nfs/nfs42proc.c
> > > > > > > > @@ -1389,7 +1389,13 @@ static int _nfs42_proc_removexattr(struct inode
> > > > > > > > *inode, const char *name)
> > > > > > > >             &res.seq_res, 1);
> > > > > > > >         trace_nfs4_removexattr(inode, name, ret);
> > > > > > > >         if (!ret)
> > > > > > > > -               nfs4_update_changeattr(inode, &res.cinfo, timestamp, 0);
> > > > > > > > +               if (nfs_have_delegated_attributes(inode)) {
> > > > > > > > +                       nfs_update_delegated_mtime(inode);
> > > > > > > > +                       spin_lock(&inode->i_lock);
> > > > > > > > +                       nfs_set_cache_invalid(inode, NFS_INO_INVALID_BLOCKS);
> > > > > > > > +                       spin_unlock(&inode->i_lock);
> > > > > > > > +               } else
> > > > > > > > +                       nfs4_update_changeattr(inode, &res.cinfo, timestamp, 0);
> > > > > > > >
> > > > > > > >         return ret;
> > > > > > > >  }
> > > > > > > >
> > > > > > >
> > > > > > > What's the advantage of doing it this way?
> > > > > > >
> > > > > > > You just sent a REMOVEXATTR operation to the server that will change
> > > > > > > the mtime there. The server has the most up-to-date version of the
> > > > > > > mtime and ctime at that point.
> > > > > >
> > > > > > In presence of delegated attributes, Is the server required to update
> > > > > > its mtime/ctime on an operation? As I mentioned, the linux server does
> > > > > > not update its ctime/mtime for WRITE, CLONE, COPY.
> > > > > >
> > > > > > Is possible that
> > > > > > some implementations might be different and also do not update the
> > > > > > ctime/mtime on REMOVEXATTR?
> > > > > >
> > > > > > Therefore I was suggesting that the patch
> > > > > > relies on the fact that it would receive an updated value. Of course
> > > > > > perhaps all implementations are done the same as the linux server and
> > > > > > my point is moot. I didn't see anything in the spec that clarifies
> > > > > > what the server supposed to do (and client rely on).
> > > > > >
> > > > >
> > > > > (cc'ing Tom)
> > > > >
> > > > > That is a very good point.
> > > > >
> > > > > My interpretation was that delegated timestamps generally covered
> > > > > writes, but SETATTR style operations that do anything beyond only
> > > > > changing the mtime can't be cached.
> > > > >
> > > > > We probably need some delstid spec clarification: for what operations
> > > > > is the server required to disable timestamp updates when a write
> > > > > delegation is outstanding?
> > > > >
> > > > > In the case of nfsd, we disable timestamp updates for WRITE/COPY/CLONE
> > > > > but not SETATTR/SETXATTR/REMOVEXATTR.
> > > > >
> > > > > How does the Hammerspace anvil behave? Does it disable c/mtime updates
> > > > > for writes when there is an outstanding timestamp delegation like we're
> > > > > doing in nfsd? If so, does it do the same for
> > > > > SETATTR/SETXATTR/REMOVEXATTR operations as well?
> > > >
> > > > Jeff,
> > > >
> > > > I think the right way to look at this is closer to how size is
> > > > handled under delegation in RFC8881, rather than as a per-op rule.
> > > >
> > > > In our implementation, because we are acting as an MDS and data I/O
> > > > goes to DSes, we already treat size as effectively delegated when
> > > > a write layout is outstanding. The MDS does not maintain authoritative
> > > > size locally in that case. We may refresh size/timestamps internally
> > > > (e.g., on GETATTR by querying DSes), but we don’t treat that as
> > > > overriding the delegated authority.
> > > >
> > > > For timestamps, our behavior is effectively the same model. When
> > > > the client holds the relevant delegation, the server does not
> > > > consider itself authoritative for ctime/mtime. If current values
> > > > are needed, we can obtain them from the client (e.g., via CB_GETATTR),
> > > > and the client must present the delegation stateid to demonstrate
> > > > that authority. So the authority follows the delegation, not the
> > > > specific operation.
> > >
> > > What happens when the client holding the attribute delegation (it's
> > > the authority) is doing the query? Is it the server's responsibility
> > > to query the client before replying.
> >
> > The Hammerspace server on a getattr checks to see if there is
> > a delegated stateid (and whether it is the client making the request
> > or not). If it is and the GETATTR asks for FATTR4_SIZE, FATTR4_TIME_MODIFY,
> > or FATTR4_TIME_METADATA, then it will send a CB_GETATTR before
> > responding to the client.
> 
> So... while I might not be running the latest Hammerspace bits and
> something has changed but I do not see Hammerspace server sending a
> CB_GETATTR when the "client is making a getattr request with a
> delegated stateid".

Or the process is broken on the server side.


> Example is the CLONE operation with an accompanied
> GETATTR. The server in this case updates the change attribute and
> mtime and returns different from what the client had previously in the
> OPEN back to the client (this is what the test expect but I think
> that's contradictory to what is said above). (To contrast nfsd server
> does not update the change attribute or mtime and returns the same
> value and thus making xfstest fail). So if the spec mandates that
> before answering the GETATTR a CB_GETATTR needs to be sent then
> neither of the server (Hammerspace nor linux) do that. But then again
> I'm not sure the client is not at fault for sending a GETATTR in the
> CLONE compound in the first place. For instance client does not have a
> GETATTR in the COPY compound (and then fails to pass xfstest that
> checks for modifies ctime/mtime). But the solution isn't clear to is
> it like Jeff's REMOVEXATTR approach to add the GETATTR to the COPY
> compound but that then should trigger a CB_GETATTR back to the client.
> Again probably none of the servers do that...
> 
> Another point I would like to raise is: doesn't it seem
> counter-intuitive to be in a situation where we have a
> delegation-holding client sending a GETATTR and then a server needing
> to do a CB_GETTATTR to the said client to fulfill the request.
> Delegations were supposed to reduce traffic and now we are introducing
> additional traffic instead. I guess I'm arguing that the client is
> broken to ever query for change_attr, mtime if it's holding the
> delegation. Yet the linux client (I could be wrong here) is written
> such that change_attr or mtime has to always come from the server. Say
> the application did a write() followed by a stat(). Client can't
> satisfy stat() without reaching out to the server. Yet the server is
> not the authority and before it can generate the reply for
> change_attr, mtime, it needs to callback to the client (CB_GETATTR).
> It seems it would have been better to never get a delegated attribute
> delegation in the first place?

Or the client needs to change its behaviour in this scenario? Why ask
as question to a source which is not the authority?

Or, I agree with your analysis...

> 
> > While it does not do so, if the client sent in a SETATTR in the
> > same compound we could short circuit that. Think a sort of WCC.
> >
> > > Example, client sends a CLONE
> > > operation which has a GETATTR attached. (1) is the server supposed to
> > > issue a CB_GETATTR before replying to the compound? (2) is the client
> > > not supposed to send any GETATTRs while holding an attribute
> > > delegation? CLONE is a modifying operation but client hasn't done any
> > > actual modifications to the opened file so a CB_GETATTR would return
> > > that file hasn't been modified. Is the server then not going to
> > > express that the file has indeed been modified when replying to
> > > GETATTR?
> >
> > The server could argue that the client wants to know what it thinks.
> > But that isn't the argeement. The server has to query those values
> > before sending them on.
> >
> > >
> > > > That said, I don’t think we’ve fully resolved the semantics for all
> > > > metadata-style ops either. WRITE and SETATTR are clear in our model,
> > > > but for things like CLONE/COPY/SETXATTR/REMOVEXATTR, we’ve likely
> > > > been relying on assumptions rather than a fully consistent rule.
> > > > I.e., CLONE and COPY we just pass through to the DS and we don't
> > > > implement SETXATTR/REMOVEXATTR.
> > > >
> > > > So the spec question, as I see it, is not whether REMOVEXATTR (or
> > > > any particular op) should update ctime/mtime, but whether delegated
> > > > timestamps are meant to follow the same attribute-authority model
> > > > as delegated size in RFC8881. If so, then we expect that the server
> > > > should query the client via CB_GETATTR to return updated ctime/mtime
> > > > after such operations while the delegation is outstanding.
> > > >
> > > > Thanks,
> > > > Tom
> > > >
> > > >
> > > > >
> > > > >
> > > > >
> > > > > > > It's certainly possible that the REMOVEXATTR is the only change that
> > > > > > > occurred. With what I'm proposing, we don't even need to do a SETATTR
> > > > > > > at all if nothing else changed. With your version, you would.
> > > > > > >
> > > > > > > > >
> > > > > > > > > Fixes: 3e1f02123fba ("NFSv4.2: add client side XDR handling for extended attributes")
> > > > > > > > > Reported-by: Olga Kornievskaia <aglo@umich.edu>
> > > > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > > > > > ---
> > > > > > > > >  fs/nfs/nfs42proc.c      | 18 ++++++++++++++++--
> > > > > > > > >  fs/nfs/nfs42xdr.c       | 10 ++++++++--
> > > > > > > > >  include/linux/nfs_xdr.h |  3 +++
> > > > > > > > >  3 files changed, 27 insertions(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> > > > > > > > > index 7b3ca68fb4bb3bee293f8621e5ed5fa596f5da3f..7e5c1172fc11c9d5a55b3621977ac83bb98f7c20 100644
> > > > > > > > > --- a/fs/nfs/nfs42proc.c
> > > > > > > > > +++ b/fs/nfs/nfs42proc.c
> > > > > > > > > @@ -1372,11 +1372,15 @@ int nfs42_proc_clone(struct file *src_f, struct file *dst_f,
> > > > > > > > >  static int _nfs42_proc_removexattr(struct inode *inode, const char *name)
> > > > > > > > >  {
> > > > > > > > >         struct nfs_server *server = NFS_SERVER(inode);
> > > > > > > > > +       __u32 bitmask[NFS_BITMASK_SZ];
> > > > > > > > >         struct nfs42_removexattrargs args = {
> > > > > > > > >                 .fh = NFS_FH(inode),
> > > > > > > > > +               .bitmask = bitmask,
> > > > > > > > >                 .xattr_name = name,
> > > > > > > > >         };
> > > > > > > > > -       struct nfs42_removexattrres res;
> > > > > > > > > +       struct nfs42_removexattrres res = {
> > > > > > > > > +               .server = server,
> > > > > > > > > +       };
> > > > > > > > >         struct rpc_message msg = {
> > > > > > > > >                 .rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_REMOVEXATTR],
> > > > > > > > >                 .rpc_argp = &args,
> > > > > > > > > @@ -1385,12 +1389,22 @@ static int _nfs42_proc_removexattr(struct inode *inode, const char *name)
> > > > > > > > >         int ret;
> > > > > > > > >         unsigned long timestamp = jiffies;
> > > > > > > > >
> > > > > > > > > +       res.fattr = nfs_alloc_fattr();
> > > > > > > > > +       if (!res.fattr)
> > > > > > > > > +               return -ENOMEM;
> > > > > > > > > +
> > > > > > > > > +       nfs4_bitmask_set(bitmask, server->cache_consistency_bitmask,
> > > > > > > > > +                        inode, NFS_INO_INVALID_CHANGE);
> > > > > > > > > +
> > > > > > > > >         ret = nfs4_call_sync(server->client, server, &msg, &args.seq_args,
> > > > > > > > >             &res.seq_res, 1);
> > > > > > > > >         trace_nfs4_removexattr(inode, name, ret);
> > > > > > > > > -       if (!ret)
> > > > > > > > > +       if (!ret) {
> > > > > > > > >                 nfs4_update_changeattr(inode, &res.cinfo, timestamp, 0);
> > > > > > > > > +               ret = nfs_post_op_update_inode(inode, res.fattr);
> > > > > > > > > +       }
> > > > > > > > >
> > > > > > > > > +       kfree(res.fattr);
> > > > > > > > >         return ret;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
> > > > > > > > > index 5c7452ce6e8ac94bd24bc3a33d4479d458a29907..ec105c62f721cfe01bfc60f5981396958084d627 100644
> > > > > > > > > --- a/fs/nfs/nfs42xdr.c
> > > > > > > > > +++ b/fs/nfs/nfs42xdr.c
> > > > > > > > > @@ -263,11 +263,13 @@
> > > > > > > > >  #define NFS4_enc_removexattr_sz                (compound_encode_hdr_maxsz + \
> > > > > > > > >                                          encode_sequence_maxsz + \
> > > > > > > > >                                          encode_putfh_maxsz + \
> > > > > > > > > -                                        encode_removexattr_maxsz)
> > > > > > > > > +                                        encode_removexattr_maxsz + \
> > > > > > > > > +                                        encode_getattr_maxsz)
> > > > > > > > >  #define NFS4_dec_removexattr_sz                (compound_decode_hdr_maxsz + \
> > > > > > > > >                                          decode_sequence_maxsz + \
> > > > > > > > >                                          decode_putfh_maxsz + \
> > > > > > > > > -                                        decode_removexattr_maxsz)
> > > > > > > > > +                                        decode_removexattr_maxsz + \
> > > > > > > > > +                                        decode_getattr_maxsz)
> > > > > > > > >
> > > > > > > > >  /*
> > > > > > > > >   * These values specify the maximum amount of data that is not
> > > > > > > > > @@ -869,6 +871,7 @@ static void nfs4_xdr_enc_removexattr(struct rpc_rqst *req,
> > > > > > > > >         encode_sequence(xdr, &args->seq_args, &hdr);
> > > > > > > > >         encode_putfh(xdr, args->fh, &hdr);
> > > > > > > > >         encode_removexattr(xdr, args->xattr_name, &hdr);
> > > > > > > > > +       encode_getfattr(xdr, args->bitmask, &hdr);
> > > > > > > > >         encode_nops(&hdr);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > @@ -1818,6 +1821,9 @@ static int nfs4_xdr_dec_removexattr(struct rpc_rqst *req,
> > > > > > > > >                 goto out;
> > > > > > > > >
> > > > > > > > >         status = decode_removexattr(xdr, &res->cinfo);
> > > > > > > > > +       if (status)
> > > > > > > > > +               goto out;
> > > > > > > > > +       status = decode_getfattr(xdr, res->fattr, res->server);
> > > > > > > > >  out:
> > > > > > > > >         return status;
> > > > > > > > >  }
> > > > > > > > > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> > > > > > > > > index ff1f12aa73d27b6fd874baf7019dd03195fc36e5..fcbd21b5685f46136a210c8e11c20a54d6ed9dad 100644
> > > > > > > > > --- a/include/linux/nfs_xdr.h
> > > > > > > > > +++ b/include/linux/nfs_xdr.h
> > > > > > > > > @@ -1611,12 +1611,15 @@ struct nfs42_listxattrsres {
> > > > > > > > >  struct nfs42_removexattrargs {
> > > > > > > > >         struct nfs4_sequence_args       seq_args;
> > > > > > > > >         struct nfs_fh                   *fh;
> > > > > > > > > +       const u32                       *bitmask;
> > > > > > > > >         const char                      *xattr_name;
> > > > > > > > >  };
> > > > > > > > >
> > > > > > > > >  struct nfs42_removexattrres {
> > > > > > > > >         struct nfs4_sequence_res        seq_res;
> > > > > > > > >         struct nfs4_change_info         cinfo;
> > > > > > > > > +       struct nfs_fattr                *fattr;
> > > > > > > > > +       const struct nfs_server         *server;
> > > > > > > > >  };
> > > > > > > > >
> > > > > > > > >  #endif /* CONFIG_NFS_V4_2 */
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > 2.53.0
> > > > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Jeff Layton <jlayton@kernel.org>
> > > > >
> > > > > --
> > > > > Jeff Layton <jlayton@kernel.org>
> > > >