diff mbox series

[03/17] xfs: use xfs_attr_defer_parent for calling xfs_attr_set on pptrs

Message ID 171323029234.253068.15430807629732077593.stgit@frogsfrogsfrogs (mailing list archive)
State New
Headers show
Series [01/17] xfs: remove some boilerplate from xfs_attr_set | expand

Commit Message

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

Adapt the xfs_attr_set function so that repair code can use it to fix
broken parent pointers.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Comments

Christoph Hellwig April 16, 2024, 5:29 a.m. UTC | #1 
On Mon, Apr 15, 2024 at 06:36:43PM -0700, Darrick J. Wong wrote:
> +			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);

> +		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);

> +		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);

Given how xfs_attr_defer_add/xfs_attr_defer_parent are basically
duplicates except for setting op_flags, shouldn't this move into
xfs_attr_defer_add?
Darrick J. Wong April 16, 2024, 4:05 p.m. UTC | #2 
On Mon, Apr 15, 2024 at 10:29:25PM -0700, Christoph Hellwig wrote:
> On Mon, Apr 15, 2024 at 06:36:43PM -0700, Darrick J. Wong wrote:
> > +			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);
> 
> > +		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);
> 
> > +		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);
> 
> Given how xfs_attr_defer_add/xfs_attr_defer_parent are basically
> duplicates except for setting op_flags, shouldn't this move into
> xfs_attr_defer_add?

I prefer to keep the pptr version separate so that we can assert that
the args contents for parent pointers is really correct.  Looking at
xfs_attr_defer_parent again, it also ought to be checking that
args->valuelen == sizeof(xfs_getparents_rec);

--D
Christoph Hellwig April 16, 2024, 4:28 p.m. UTC | #3 
On Tue, Apr 16, 2024 at 09:05:55AM -0700, Darrick J. Wong wrote:
> I prefer to keep the pptr version separate so that we can assert that
> the args contents for parent pointers is really correct.

We could do that with a merged version just as easily.

> Looking at
> xfs_attr_defer_parent again, it also ought to be checking that
> args->valuelen == sizeof(xfs_getparents_rec);

Yes.
Darrick J. Wong April 16, 2024, 6:41 p.m. UTC | #4 
On Tue, Apr 16, 2024 at 09:28:09AM -0700, Christoph Hellwig wrote:
> On Tue, Apr 16, 2024 at 09:05:55AM -0700, Darrick J. Wong wrote:
> > I prefer to keep the pptr version separate so that we can assert that
> > the args contents for parent pointers is really correct.
> 
> We could do that with a merged version just as easily.

Eh, ok.

> > Looking at
> > xfs_attr_defer_parent again, it also ought to be checking that
> > args->valuelen == sizeof(xfs_getparents_rec);
> 
> Yes.

The patch "xfs: create attr log item opcodes and formats for parent
pointers" now contains:

void
xfs_attr_defer_add(
	struct xfs_da_args	*args,
	enum xfs_attr_defer_op	op)
{
	struct xfs_attr_intent	*new;
	unsigned int		log_op = 0;
	bool			is_pptr = args->attr_filter & XFS_ATTR_PARENT;

	if (is_pptr) {
		ASSERT(xfs_has_parent(args->dp->i_mount));
		ASSERT((args->attr_filter & ~XFS_ATTR_PARENT) != 0);
		ASSERT(args->op_flags & XFS_DA_OP_LOGGED);
		ASSERT(args->valuelen == sizeof(struct xfs_parent_rec));
	}

	new = kmem_cache_zalloc(xfs_attr_intent_cache,
			GFP_NOFS | __GFP_NOFAIL);
	new->xattri_da_args = args;

	/* Compute log operation from the higher level op and namespace. */
	switch (op) {
	case XFS_ATTR_DEFER_SET:
		if (is_pptr)
			log_op = XFS_ATTRI_OP_FLAGS_PPTR_SET;
		else
			log_op = XFS_ATTRI_OP_FLAGS_SET;
		break;
	case XFS_ATTR_DEFER_REPLACE:
		if (is_pptr)
			log_op = XFS_ATTRI_OP_FLAGS_PPTR_REPLACE;
		else
			log_op = XFS_ATTRI_OP_FLAGS_REPLACE;
		break;
	case XFS_ATTR_DEFER_REMOVE:
		if (is_pptr)
			log_op = XFS_ATTRI_OP_FLAGS_PPTR_REMOVE;
		else
			log_op = XFS_ATTRI_OP_FLAGS_REMOVE;
		break;
	default:
		ASSERT(0);
		break;
	}
	new->xattri_op_flags = log_op;

	/* Set up initial attr operation state. */
	switch (log_op) {
	case XFS_ATTRI_OP_FLAGS_PPTR_SET:
	case XFS_ATTRI_OP_FLAGS_SET:
		new->xattri_dela_state = xfs_attr_init_add_state(args);
		break;
	case XFS_ATTRI_OP_FLAGS_PPTR_REPLACE:
		ASSERT(args->new_valuelen == args->valuelen);
		new->xattri_dela_state = xfs_attr_init_replace_state(args);
		break;
	case XFS_ATTRI_OP_FLAGS_REPLACE:
		new->xattri_dela_state = xfs_attr_init_replace_state(args);
		break;
	case XFS_ATTRI_OP_FLAGS_PPTR_REMOVE:
	case XFS_ATTRI_OP_FLAGS_REMOVE:
		new->xattri_dela_state = xfs_attr_init_remove_state(args);
		break;
	}

	xfs_defer_add(args->trans, &new->xattri_list, &xfs_attr_defer_type);
	trace_xfs_attr_defer_add(new->xattri_dela_state, args->dp);
}

and then we can drop this patch.

--D
Christoph Hellwig April 16, 2024, 6:51 p.m. UTC | #5 
This looks sensible to me.
Darrick J. Wong April 17, 2024, 2:54 a.m. UTC | #6 
On Tue, Apr 16, 2024 at 11:51:28AM -0700, Christoph Hellwig wrote:
> This looks sensible to me.

Ok.  I merged the two functions in the patch where we introduce the new
pptr log op codes, because that made more sense to me:

https://lore.kernel.org/linux-xfs/20240417025208.GM11948@frogsfrogsfrogs/

--D
Christoph Hellwig April 17, 2024, 5 a.m. UTC | #7 
On Tue, Apr 16, 2024 at 07:54:07PM -0700, Darrick J. Wong wrote:
> On Tue, Apr 16, 2024 at 11:51:28AM -0700, Christoph Hellwig wrote:
> > This looks sensible to me.
> 
> Ok.  I merged the two functions in the patch where we introduce the new
> pptr log op codes, because that made more sense to me:
> 
> https://lore.kernel.org/linux-xfs/20240417025208.GM11948@frogsfrogsfrogs/

Yes, this looks sensible to me.
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index c98145596f029..b18f6c258a174 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1027,14 +1027,21 @@  xfs_attr_set(
 	case -EEXIST:
 		if (op == XFS_ATTRUPDATE_REMOVE) {
 			/* if no value, we are performing a remove operation */
-			xfs_attr_defer_add(args, XFS_ATTR_DEFER_REMOVE);
+			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);
 			break;
 		}
 
 		/* Pure create fails if the attr already exists */
 		if (op == XFS_ATTRUPDATE_CREATE)
 			goto out_trans_cancel;
-		xfs_attr_defer_add(args, XFS_ATTR_DEFER_REPLACE);
+		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:
 		/* Can't remove what isn't there. */
@@ -1044,7 +1051,10 @@  xfs_attr_set(
 		/* Pure replace fails if no existing attr to replace. */
 		if (op == XFS_ATTRUPDATE_REPLACE)
 			goto out_trans_cancel;
-		xfs_attr_defer_add(args, XFS_ATTR_DEFER_SET);
+		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;