Re: [PATCH 01/14] xfs: add xattr setname and removename functions for internal users

Darrick J. Wong
2024-04-10T22:18:44Z

On Tue, Apr 09, 2024 at 11:18:03PM -0700, Christoph Hellwig wrote:
> > +static int
> > +xfs_attr_ensure_iext(
> > + struct xfs_da_args *args,
> > + int nr)
> > +{
> > + int error;
> > +
> > + error = xfs_iext_count_may_overflow(args->dp, XFS_ATTR_FORK, nr);
> > + if (error == -EFBIG)
> > + return xfs_iext_count_upgrade(args->trans, args->dp, nr);
> > + return error;
> > +}
>
> I'd rather get my consolidation of these merged instead of adding
> a wrapper like this. Just waiting for my RT delalloc and your
> exchrange series to hit for-next to resend it.

Yeah, I made a mental note to scrub this function out if your patch wins
the race.

> > +/*
> > + * Ensure that the xattr structure maps @args->name to @args->value.
> > + *
> > + * The caller must have initialized @args, attached dquots, and must not hold
> > + * any ILOCKs. Only XATTR_CREATE may be specified in @args->xattr_flags.
> > + * Reserved data blocks may be used if @rsvd is set.
> > + *
> > + * Returns -EEXIST if XATTR_CREATE was specified and the name already exists.
> > + */
> > +int
> > +xfs_attr_setname(
>
> Is there any case where we do not want to pass XATTR_CREATE, that
> is replace an existing attribute when there is one?

Yes, verity setup will use xfs_attr_setname to upsert a merkle tree
block into the attr structure and obliterate stale blocks that might
already have been there.

> > +int
> > +xfs_attr_removename(
> > + struct xfs_da_args *args,
> > + bool rsvd)
> > +{
>
> Is there a good reason to have a separate remove helper and not
> overload a NULL value like we do for the normal xattr interface?

xfs_repair uses xfs_parent_unset -> xfs_attr_removename to erase any
XFS_ATTR_PARENT attribute that doesn't validate, so it needs to be able
to pass in a non-NULL value. Perhaps I'll add a comment about that,
since this isn't the first time this has come up.

Come to think of it you can't removename a remote parent value, so I
guess in that bad case xfs_repair will have to drop the entire attr
structure <frown>.

/*
* Ensure that the xattr structure does not map @args->name to @args->value.
* @args->value must be set for XFS_ATTR_PARENT removal (e.g. xfs_repair).
*
* The caller must have initialized @args, attached dquots, and must not hold
* any ILOCKs. Reserved data blocks may be used if @rsvd is set.
*
* Returns -ENOATTR if the name did not already exist.
*/

--D