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

[Thomas Haynes]

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@xxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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.

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@xxxxxxxxx>
> > > > > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > > > > > ---
> > > > > > >  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@xxxxxxxxxx>
> > >
> > > --
> > > Jeff Layton <jlayton@xxxxxxxxxx>
> >