diff mbox series

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

Message ID 171270971004.3632937.5852027532367765797.stgit@frogsfrogsfrogs (mailing list archive)
State New
Headers show
Series [01/14] xfs: add xattr setname and removename functions for internal users | expand

Commit Message

Darrick J. Wong April 10, 2024, 1:03 a.m. UTC
From: Darrick J. Wong <djwong@kernel.org>

Add a couple of internal xattr functions to set or remove attr names
from the xattr structures.  The upcoming parent pointer and fsverity
patchsets will want the ability to set and clear xattrs with a fully
initialized xfs_da_args structure.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c   |  193 ++++++++++++++++++++++++++++++++++++++++----
 fs/xfs/libxfs/xfs_attr.h   |    3 +
 fs/xfs/scrub/attr_repair.c |   17 +++-
 3 files changed, 191 insertions(+), 22 deletions(-)

Comments

Christoph Hellwig April 10, 2024, 6:18 a.m. UTC | #1
> +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.

> +/*
> + * 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?

> +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?
Darrick J. Wong April 10, 2024, 10:18 p.m. UTC | #2
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
Christoph Hellwig April 11, 2024, 3:32 a.m. UTC | #3
On Wed, Apr 10, 2024 at 03:18:44PM -0700, Darrick J. Wong wrote:
> > 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>.

Maybe we'll need to fix that.  How about you leave the xattr_flags in
place for now, and then I or you if you really want) replace it with
a new enum argument:

enum xfs_attr_change {
	XFS_ATTR_CREATE,
	XFS_ATTR_REPLACE,
	XFS_ATTR_CREATE_OR_REPLACE,
	XFS_ATTR_REMOVE,
};

and we pass that to xfs_attr_set and what is current xfs_attr_setname
(which btw is a name that feels really odd).  That way repair can
also use the libxfs attr helpers with a value match for parent pointers?
Darrick J. Wong April 11, 2024, 4:30 a.m. UTC | #4
On Wed, Apr 10, 2024 at 08:32:44PM -0700, Christoph Hellwig wrote:
> On Wed, Apr 10, 2024 at 03:18:44PM -0700, Darrick J. Wong wrote:
> > > 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>.
> 
> Maybe we'll need to fix that.  How about you leave the xattr_flags in
> place for now, and then I or you if you really want) replace it with
> a new enum argument:
> 
> enum xfs_attr_change {
> 	XFS_ATTR_CREATE,
> 	XFS_ATTR_REPLACE,
> 	XFS_ATTR_CREATE_OR_REPLACE,
> 	XFS_ATTR_REMOVE,
> };

Heh, I almost did that:

enum xfs_attr_change {
	XAC_CREATE	= XATTR_CREATE,
	XAC_REPLACE	= XATTR_REPLACE,
	XAC_UPSERT,
	XAC_REMOVE,
};

(500 patches from now when I get around to removing xattr_flags & making
it a parameter.)

> and we pass that to xfs_attr_set and what is current xfs_attr_setname
> (which btw is a name that feels really odd).  That way repair can
> also use the libxfs attr helpers with a value match for parent pointers?

I think this is a good idea.  Maybe even worth rebasing through the
tree.

--D
Christoph Hellwig April 11, 2024, 4:50 a.m. UTC | #5
On Wed, Apr 10, 2024 at 09:30:48PM -0700, Darrick J. Wong wrote:
> Heh, I almost did that:
> 
> enum xfs_attr_change {
> 	XAC_CREATE	= XATTR_CREATE,
> 	XAC_REPLACE	= XATTR_REPLACE,
> 	XAC_UPSERT,
> 	XAC_REMOVE,
> };
> 
> (500 patches from now when I get around to removing xattr_flags & making
> it a parameter.)

Heh. Reusing the XATTR_* values is ok, but I doubt it's really worth
the effort given that just one caller actually uses them.
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 99930472e59da..83f8cf551816a 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -950,6 +950,44 @@  xfs_attr_lookup(
 	return error;
 }
 
+/*
+ * Before updating xattrs, add an attribute fork if the inode doesn't have.
+ * (inode must not be locked when we call this routine)
+ */
+static int
+xfs_attr_ensure_fork(
+	struct xfs_da_args	*args,
+	bool			rsvd)
+{
+	int			sf_size;
+
+	if (xfs_inode_has_attr_fork(args->dp))
+		return 0;
+
+	sf_size = sizeof(struct xfs_attr_sf_hdr) +
+			xfs_attr_sf_entsize_byname(args->namelen,
+						   args->valuelen);
+
+	return xfs_bmap_add_attrfork(args->dp, sf_size, rsvd);
+}
+
+/*
+ * Before updating xattrs, make sure we can handle adding to the extent count.
+ * There must be a transaction and the ILOCK must be held.
+ */
+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;
+}
+
 /*
  * Note: If args->value is NULL the attribute will be removed, just like the
  * Linux ->setattr API.
@@ -994,19 +1032,9 @@  xfs_attr_set(
 		XFS_STATS_INC(mp, xs_attr_set);
 		args->total = xfs_attr_calc_size(args, &local);
 
-		/*
-		 * If the inode doesn't have an attribute fork, add one.
-		 * (inode must not be locked when we call this routine)
-		 */
-		if (xfs_inode_has_attr_fork(dp) == 0) {
-			int sf_size = sizeof(struct xfs_attr_sf_hdr) +
-				xfs_attr_sf_entsize_byname(args->namelen,
-						args->valuelen);
-
-			error = xfs_bmap_add_attrfork(dp, sf_size, rsvd);
-			if (error)
-				return error;
-		}
+		error = xfs_attr_ensure_fork(args, rsvd);
+		if (error)
+			return error;
 
 		if (!local)
 			rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
@@ -1025,11 +1053,8 @@  xfs_attr_set(
 		return error;
 
 	if (args->value || xfs_inode_hasattr(dp)) {
-		error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
+		error = xfs_attr_ensure_iext(args,
 				XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
-		if (error == -EFBIG)
-			error = xfs_iext_count_upgrade(args->trans, dp,
-					XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
 		if (error)
 			goto out_trans_cancel;
 	}
@@ -1086,6 +1111,140 @@  xfs_attr_set(
 	goto out_unlock;
 }
 
+/*
+ * 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(
+	struct xfs_da_args	*args,
+	bool			rsvd)
+{
+	struct xfs_inode	*dp = args->dp;
+	struct xfs_mount	*mp = dp->i_mount;
+	struct xfs_trans_res	tres;
+	unsigned int		total;
+	int			rmt_extents = 0;
+	int			error, local;
+
+	ASSERT(!(args->xattr_flags & XATTR_REPLACE));
+	ASSERT(!args->trans);
+
+	args->total = xfs_attr_calc_size(args, &local);
+
+	error = xfs_attr_ensure_fork(args, rsvd);
+	if (error)
+		return error;
+
+	if (!local)
+		rmt_extents = XFS_IEXT_ATTR_MANIP_CNT(
+				xfs_attr3_rmt_blocks(mp, args->valuelen));
+
+	xfs_init_attr_trans(args, &tres, &total);
+	error = xfs_trans_alloc_inode(dp, &tres, total, 0, rsvd, &args->trans);
+	if (error)
+		return error;
+
+	error = xfs_attr_ensure_iext(args, rmt_extents);
+	if (error)
+		goto out_trans_cancel;
+
+	error = xfs_attr_lookup(args);
+	switch (error) {
+	case -EEXIST:
+		/* Pure create fails if the attr already exists */
+		if (args->xattr_flags & XATTR_CREATE)
+			goto out_trans_cancel;
+		if (args->attr_filter & XFS_ATTR_PARENT)
+			xfs_attr_defer_parent(args, XFS_ATTR_DEFER_REPLACE);
+		else
+			xfs_attr_defer_add(args, XFS_ATTR_DEFER_REPLACE);
+		break;
+	case -ENOATTR:
+		if (args->attr_filter & XFS_ATTR_PARENT)
+			xfs_attr_defer_parent(args, XFS_ATTR_DEFER_SET);
+		else
+			xfs_attr_defer_add(args, XFS_ATTR_DEFER_SET);
+		break;
+	default:
+		goto out_trans_cancel;
+	}
+
+	xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
+	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE);
+	error = xfs_trans_commit(args->trans);
+out_unlock:
+	args->trans = NULL;
+	xfs_iunlock(dp, XFS_ILOCK_EXCL);
+	return error;
+
+out_trans_cancel:
+	xfs_trans_cancel(args->trans);
+	goto out_unlock;
+}
+
+/*
+ * Ensure that the xattr structure does not map @args->name to @args->value.
+ *
+ * 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.
+ */
+int
+xfs_attr_removename(
+	struct xfs_da_args	*args,
+	bool			rsvd)
+{
+	struct xfs_inode	*dp = args->dp;
+	struct xfs_mount	*mp = dp->i_mount;
+	struct xfs_trans_res	tres;
+	unsigned int		total;
+	int			rmt_extents;
+	int			error;
+
+	ASSERT(!args->trans);
+
+	rmt_extents = XFS_IEXT_ATTR_MANIP_CNT(
+				xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX));
+
+	xfs_init_attr_trans(args, &tres, &total);
+	error = xfs_trans_alloc_inode(dp, &tres, total, 0, rsvd, &args->trans);
+	if (error)
+		return error;
+
+	if (xfs_inode_hasattr(dp)) {
+		error = xfs_attr_ensure_iext(args, rmt_extents);
+		if (error)
+			goto out_trans_cancel;
+	}
+
+	error = xfs_attr_lookup(args);
+	if (error != -EEXIST)
+		goto out_trans_cancel;
+
+	if (args->attr_filter & XFS_ATTR_PARENT)
+		xfs_attr_defer_parent(args, XFS_ATTR_DEFER_REMOVE);
+	else
+		xfs_attr_defer_add(args, XFS_ATTR_DEFER_REMOVE);
+	xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
+	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE);
+	error = xfs_trans_commit(args->trans);
+out_unlock:
+	args->trans = NULL;
+	xfs_iunlock(dp, XFS_ILOCK_EXCL);
+	return error;
+
+out_trans_cancel:
+	xfs_trans_cancel(args->trans);
+	goto out_unlock;
+}
+
 /*========================================================================
  * External routines when attribute list is inside the inode
  *========================================================================*/
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index cb5ca37000848..d51001c5809fe 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -560,6 +560,9 @@  int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
 void xfs_init_attr_trans(struct xfs_da_args *args, struct xfs_trans_res *tres,
 			 unsigned int *total);
 
+int xfs_attr_setname(struct xfs_da_args *args, bool rsvd);
+int xfs_attr_removename(struct xfs_da_args *args, bool rsvd);
+
 /*
  * Check to see if the attr should be upgraded from non-existent or shortform to
  * single-leaf-block attribute list.
diff --git a/fs/xfs/scrub/attr_repair.c b/fs/xfs/scrub/attr_repair.c
index 091cef077cdde..a3a98051df0fb 100644
--- a/fs/xfs/scrub/attr_repair.c
+++ b/fs/xfs/scrub/attr_repair.c
@@ -570,6 +570,9 @@  xrep_xattr_insert_rec(
 		.namelen		= key->namelen,
 		.valuelen		= key->valuelen,
 		.owner			= rx->sc->ip->i_ino,
+		.geo			= rx->sc->mp->m_attr_geo,
+		.whichfork		= XFS_ATTR_FORK,
+		.op_flags		= XFS_DA_OP_OKNOENT,
 	};
 	struct xchk_xattr_buf		*ab = rx->sc->buf;
 	int				error;
@@ -607,19 +610,23 @@  xrep_xattr_insert_rec(
 
 	ab->name[key->namelen] = 0;
 
-	if (key->flags & XFS_ATTR_PARENT)
+	if (key->flags & XFS_ATTR_PARENT) {
 		trace_xrep_xattr_insert_pptr(rx->sc->tempip, key->flags,
 				ab->name, key->namelen, ab->value,
 				key->valuelen);
-	else
+		args.op_flags |= XFS_DA_OP_LOGGED;
+	} else {
 		trace_xrep_xattr_insert_rec(rx->sc->tempip, key->flags,
 				ab->name, key->namelen, key->valuelen);
+	}
 
 	/*
-	 * xfs_attr_set creates and commits its own transaction.  If the attr
-	 * already exists, we'll just drop it during the rebuild.
+	 * xfs_attr_setname creates and commits its own transaction.  If the
+	 * attr already exists, we'll just drop it during the rebuild.  Don't
+	 * use reserved blocks because we can abort the repair with ENOSPC.
 	 */
-	error = xfs_attr_set(&args);
+	xfs_attr_sethash(&args);
+	error = xfs_attr_setname(&args, false);
 	if (error == -EEXIST)
 		error = 0;