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

[Jeff Layton]

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.

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>