Re: [PATCH 13/18] xfs: introduce attr remove initial states into xfs_attr_set_iter

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, May 10, 2022 at 04:37:36PM -0700, Darrick J. Wong wrote:
> On Mon, May 09, 2022 at 10:41:33AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > We need to merge the add and remove code paths to enable safe
> > recovery of replace operations. Hoist the initial remove states from
> > xfs_attr_remove_iter into xfs_attr_set_iter. We will make use of
> > them in the next patches.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > Reviewed-by: Allison Henderson<allison.henderson@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_attr.c | 139 ++++++++++++++++++++++-----------------
> >  fs/xfs/libxfs/xfs_attr.h |   4 ++
> >  fs/xfs/xfs_trace.h       |   3 +
> >  3 files changed, 84 insertions(+), 62 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 89e68d9e22c0..a6a9b1f8dce6 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -451,6 +451,68 @@ xfs_attr_rmtval_alloc(
> >  	return error;
> >  }
> >  
> > +/*
> > + * Mark an attribute entry INCOMPLETE and save pointers to the relevant buffers
> > + * for later deletion of the entry.
> > + */
> > +static int
> > +xfs_attr_leaf_mark_incomplete(
> > +	struct xfs_da_args	*args,
> > +	struct xfs_da_state	*state)
> > +{
> > +	int			error;
> > +
> > +	/*
> > +	 * Fill in disk block numbers in the state structure
> > +	 * so that we can get the buffers back after we commit
> > +	 * several transactions in the following calls.
> > +	 */
> > +	error = xfs_attr_fillstate(state);
> > +	if (error)
> > +		return error;
> > +
> > +	/*
> > +	 * Mark the attribute as INCOMPLETE
> > +	 */
> > +	return xfs_attr3_leaf_setflag(args);
> > +}
> > +
> > +/*
> > + * Initial setup for xfs_attr_node_removename.  Make sure the attr is there and
> > + * the blocks are valid.  Attr keys with remote blocks will be marked
> > + * incomplete.
> > + */
> > +static
> > +int xfs_attr_node_removename_setup(
> > +	struct xfs_attr_item		*attr)
> > +{
> > +	struct xfs_da_args		*args = attr->xattri_da_args;
> > +	struct xfs_da_state		**state = &attr->xattri_da_state;
> > +	int				error;
> > +
> > +	error = xfs_attr_node_hasname(args, state);
> > +	if (error != -EEXIST)
> > +		goto out;
> > +	error = 0;
> > +
> > +	ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL);
> > +	ASSERT((*state)->path.blk[(*state)->path.active - 1].magic ==
> > +		XFS_ATTR_LEAF_MAGIC);
> > +
> > +	if (args->rmtblkno > 0) {
> > +		error = xfs_attr_leaf_mark_incomplete(args, *state);
> > +		if (error)
> > +			goto out;
> > +
> > +		error = xfs_attr_rmtval_invalidate(args);
> > +	}
> > +out:
> > +	if (error)
> > +		xfs_da_state_free(*state);
> > +
> > +	return error;
> > +}
> > +
> >  /*
> >   * Remove the original attr we have just replaced. This is dependent on the
> >   * original lookup and insert placing the old attr in args->blkno/args->index
> > @@ -550,6 +612,21 @@ xfs_attr_set_iter(
> >  	case XFS_DAS_NODE_ADD:
> >  		return xfs_attr_node_addname(attr);
> >  
> > +	case XFS_DAS_SF_REMOVE:
> 
> I think attr removal is broken until the next patch, which adds
> xfs_attr_init_remove_state to set dela_state to DAS_*_REMOVE, right?

Aha, I was looking at the wrong tab, the old xfs_attr_remove_iter code
is still in place, so ignore this question...

> The rest of the state machine changes look solid.
> 
> With that moved up by one patch to fix the bisection issue,
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

..and go straight to the RVB tag.

--D

> 
> --D
> 
> > +		attr->xattri_dela_state = XFS_DAS_DONE;
> > +		return xfs_attr_sf_removename(args);
> > +	case XFS_DAS_LEAF_REMOVE:
> > +		attr->xattri_dela_state = XFS_DAS_DONE;
> > +		return xfs_attr_leaf_removename(args);
> > +	case XFS_DAS_NODE_REMOVE:
> > +		error = xfs_attr_node_removename_setup(attr);
> > +		if (error)
> > +			return error;
> > +		attr->xattri_dela_state = XFS_DAS_NODE_REMOVE_RMT;
> > +		if (args->rmtblkno == 0)
> > +			attr->xattri_dela_state++;
> > +		break;
> > +
> >  	case XFS_DAS_LEAF_SET_RMT:
> >  	case XFS_DAS_NODE_SET_RMT:
> >  		error = xfs_attr_rmtval_find_space(attr);
> > @@ -1351,68 +1428,6 @@ xfs_attr_node_remove_attr(
> >  }
> >  
> >  
> > -/*
> > - * Mark an attribute entry INCOMPLETE and save pointers to the relevant buffers
> > - * for later deletion of the entry.
> > - */
> > -STATIC int
> > -xfs_attr_leaf_mark_incomplete(
> > -	struct xfs_da_args	*args,
> > -	struct xfs_da_state	*state)
> > -{
> > -	int			error;
> > -
> > -	/*
> > -	 * Fill in disk block numbers in the state structure
> > -	 * so that we can get the buffers back after we commit
> > -	 * several transactions in the following calls.
> > -	 */
> > -	error = xfs_attr_fillstate(state);
> > -	if (error)
> > -		return error;
> > -
> > -	/*
> > -	 * Mark the attribute as INCOMPLETE
> > -	 */
> > -	return xfs_attr3_leaf_setflag(args);
> > -}
> > -
> > -/*
> > - * Initial setup for xfs_attr_node_removename.  Make sure the attr is there and
> > - * the blocks are valid.  Attr keys with remote blocks will be marked
> > - * incomplete.
> > - */
> > -STATIC
> > -int xfs_attr_node_removename_setup(
> > -	struct xfs_attr_item		*attr)
> > -{
> > -	struct xfs_da_args		*args = attr->xattri_da_args;
> > -	struct xfs_da_state		**state = &attr->xattri_da_state;
> > -	int				error;
> > -
> > -	error = xfs_attr_node_hasname(args, state);
> > -	if (error != -EEXIST)
> > -		goto out;
> > -	error = 0;
> > -
> > -	ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL);
> > -	ASSERT((*state)->path.blk[(*state)->path.active - 1].magic ==
> > -		XFS_ATTR_LEAF_MAGIC);
> > -
> > -	if (args->rmtblkno > 0) {
> > -		error = xfs_attr_leaf_mark_incomplete(args, *state);
> > -		if (error)
> > -			goto out;
> > -
> > -		error = xfs_attr_rmtval_invalidate(args);
> > -	}
> > -out:
> > -	if (error)
> > -		xfs_da_state_free(*state);
> > -
> > -	return error;
> > -}
> > -
> >  STATIC int
> >  xfs_attr_node_removename(
> >  	struct xfs_da_args	*args,
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index c318260f17d4..7ea7c7fa31ac 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -451,6 +451,10 @@ enum xfs_delattr_state {
> >  	XFS_DAS_RM_NAME,		/* Remove attr name */
> >  	XFS_DAS_RM_SHRINK,		/* We are shrinking the tree */
> >  
> > +	XFS_DAS_SF_REMOVE,		/* Initial shortform set iter state */
> > +	XFS_DAS_LEAF_REMOVE,		/* Initial leaf form set iter state */
> > +	XFS_DAS_NODE_REMOVE,		/* Initial node form set iter state */
> > +
> >  	/* Leaf state set/replace sequence */
> >  	XFS_DAS_LEAF_SET_RMT,		/* set a remote xattr from a leaf */
> >  	XFS_DAS_LEAF_ALLOC_RMT,		/* We are allocating remote blocks */
> > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> > index 260760ce2d05..01b047d86cd1 100644
> > --- a/fs/xfs/xfs_trace.h
> > +++ b/fs/xfs/xfs_trace.h
> > @@ -4136,6 +4136,9 @@ TRACE_DEFINE_ENUM(XFS_DAS_NODE_ADD);
> >  TRACE_DEFINE_ENUM(XFS_DAS_RMTBLK);
> >  TRACE_DEFINE_ENUM(XFS_DAS_RM_NAME);
> >  TRACE_DEFINE_ENUM(XFS_DAS_RM_SHRINK);
> > +TRACE_DEFINE_ENUM(XFS_DAS_SF_REMOVE);
> > +TRACE_DEFINE_ENUM(XFS_DAS_LEAF_REMOVE);
> > +TRACE_DEFINE_ENUM(XFS_DAS_NODE_REMOVE);
> >  TRACE_DEFINE_ENUM(XFS_DAS_LEAF_SET_RMT);
> >  TRACE_DEFINE_ENUM(XFS_DAS_LEAF_ALLOC_RMT);
> >  TRACE_DEFINE_ENUM(XFS_DAS_LEAF_REPLACE);
> > -- 
> > 2.35.1
> >