Re: [PATCH RFC 1/1] xfs: refactor xfs_attr_set follow up

[Brian Foster <bfoster@redhat.com>]

On Mon, Jan 25, 2021 at 03:22:59PM -0700, Allison Henderson wrote:
> 
> 
> On 1/21/21 11:47 AM, Brian Foster wrote:
> > On Sat, Jan 16, 2021 at 01:12:40AM -0700, Allison Henderson wrote:
> > > Hi all,
> > > 
> > > This is a follow up to Brians earlier patch
> > > "[PATCH RFC] xfs: refactor xfs_attr_set() into incremental components"
> > > 
> > > This patch resembles the earlier patch, but it is seated at the top of
> > > the parent pointers set rather than the bottom to give a better
> > > illustraion of what this approach might end up looking like in the
> > > bigger picture.  This patch is both compiled and tested, and is meant to
> > > be more of an exploratory effort than anything.
> > > 
> > > Most of the state management is collapsed into the *_iter functions
> > > similar to Brians patch which collapsed them into the *_args routines.
> > > Though there are a few states that a still in a few subfunctions.
> > > 
> > > In anycase, I think it gives decent idea of what the solution might
> > > look like in practice.  Questions, comments and feedback appreciated.
> > > 
> > 
> > Thanks for the patch. By and large, I think the centralized state
> > management of __xfs_attr_set_iter() is much more clear than the label
> > management approach of jumping up and down through multiple levels of
> > helper functions. For the most part, I'm able to walk through the iter
> > function and follow the sequence of steps involved in the set. I did
> > have some higher level comments on various parts of the patch,
> > particularly where we seem to deviate from centralized state management.
> > 
> > Note that if we were to take this approach, a primary goal was to
> > incrementally port the existing xfs_attr_set_args() implementation into
> > states. For example, such that we could split the current monstrous
> > xfs_attr_set() patch into multiple patches that introduce infrastructure
> > first, and then convert the existing code a state or so at a time. That
> > eliminates churn from factoring code into one scheme only to immediately
> > refactor into another. It also facilitates testing because I think the
> > rework should be able to maintain functionality across each step.
> > 
> > Porting on top of the whole thing certainly helps to get an advanced
> > look at the final result. However, if we do use this approach and start
> > getting into the details of individual states and whatnot, I do think it
> > would be better to start breaking things down into smaller patches that
> > replace some of the earlier code rather than use it as a baseline.
> Sure, I think doing quick and dirty patches on top moves a little faster
> just because the infastructure is already setup, and I'm not working through
> merge conflicts, plus it cuts out the userspace side and just the misc nits
> that are probably best done after the greater architectural descisions are
> set.  The idea being to just establish an end goal of course.  It is tougher
> to review like this though, maybe we can do a few spins of this and I'll try
> to break it down into smaller chunks if that's ok.
> 
> 
> > Further comments inline...
> > 
> > > Thanks!
> > > Allison
> > > 
> > > ---
> > >   fs/xfs/libxfs/xfs_attr.c | 596 +++++++++++++++++++----------------------------
> > >   fs/xfs/libxfs/xfs_attr.h |   4 +-
> > >   2 files changed, 247 insertions(+), 353 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > index 6ba8f4b..356e35c 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > +++ b/fs/xfs/libxfs/xfs_attr.c
...
> > > @@ -274,108 +300,197 @@ xfs_attr_set_shortform(
...
> > > +int xfs_attr_set_iter(
> > > +	struct xfs_attr_item	*attr)
> > > +{
> > > +	bool	done = true;
> > > +	int 	error;
> > > +
> > > +	error =  __xfs_attr_set_iter(attr, &done);
> > > +	if (error || done)
> > > +		return error;
> > > +
> > > +	return xfs_attr_node_addname(attr);
> > 
> > Note that this was also intended to go away and get folded into the
> > state code in __xfs_attr_set_iter(), I just left off here because it
> > looked like there might have been opportunity to fall into the remove
> > path, and that was getting a bit more involved than I wanted to. This
> > variant looks a little different in that we can presumably fall into
> > this function and then back into the state machine. Even if we didn't
> > immediately reuse the remove path, I suspect we should probably continue
> > chunking off the remainder of the operation into proper states.
> Yes, I'm thinking maybe I could chop this down into a few patches that just
> sort of hoist everything up and then try to collapse down any duplicated
> code in a successive patch.  It'll create bit of a monster function at
> first, but I think it might help us find the an arrangement we like.
> 

FWIW, I've seen enough that the design seems sane and reasonable to me.
It addresses my primary gripe with the current implementation of
spreading the state management code around a bit too much. My previous
comments on this patch were around some of the detailed state breakdown
and management logic, but I don't think that really requires seeing the
whole thing completely done and functional to resolve. In fact, I'd
rather not get too deep into detailed functional and state review with
an RFC patch based on top of the current implementation, as this patch
is, because that will just have to be repeated when broken down into
smaller patches and rebased.

Of course, that is not to say we shouldn't continue down this RFC path
if you have other reasons to do so, aren't sold on the approach, have
design concerns, etc. I'm not totally clear on where we stand on that
tbh. This is just an FYI that if you want my .02, I think the design is
reasonable and I'd probably reserve detailed review for a proper non-rfc
series.

Brian

> Thanks for the reviews!  I know its complicated!!
> Allison
> 
> > 
> > Brian
> > 
> > > +}
> > > +
> > >   /*
> > >    * Return EEXIST if attr is found, or ENOATTR if not
> > >    */
> > > @@ -773,145 +888,6 @@ xfs_attr_leaf_try_add(
> > >   /*
> > > - * Add a name to the leaf attribute list structure
> > > - *
> > > - * This leaf block cannot have a "remote" value, we only call this routine
> > > - * if bmap_one_block() says there is only one block (ie: no remote blks).
> > > - *
> > > - * This routine is meant to function as a delayed operation, and may return
> > > - * -EAGAIN when the transaction needs to be rolled.  Calling functions will need
> > > - * to handle this, and recall the function until a successful error code is
> > > - * returned.
> > > - */
> > > -STATIC int
> > > -xfs_attr_leaf_addname(
> > > -	struct xfs_attr_item		*attr)
> > > -{
> > > -	struct xfs_da_args		*args = attr->xattri_da_args;
> > > -	struct xfs_buf			*bp = NULL;
> > > -	int				error, forkoff;
> > > -	struct xfs_inode		*dp = args->dp;
> > > -	struct xfs_mount		*mp = args->dp->i_mount;
> > > -
> > > -	/* State machine switch */
> > > -	switch (attr->xattri_dela_state) {
> > > -	case XFS_DAS_FLIP_LFLAG:
> > > -		goto das_flip_flag;
> > > -	case XFS_DAS_RM_LBLK:
> > > -		goto das_rm_lblk;
> > > -	default:
> > > -		break;
> > > -	}
> > > -
> > > -	/*
> > > -	 * If there was an out-of-line value, allocate the blocks we
> > > -	 * identified for its storage and copy the value.  This is done
> > > -	 * after we create the attribute so that we don't overflow the
> > > -	 * maximum size of a transaction and/or hit a deadlock.
> > > -	 */
> > > -
> > > -	/* Open coded xfs_attr_rmtval_set without trans handling */
> > > -	if ((attr->xattri_flags & XFS_DAC_LEAF_ADDNAME_INIT) == 0) {
> > > -		attr->xattri_flags |= XFS_DAC_LEAF_ADDNAME_INIT;
> > > -		if (args->rmtblkno > 0) {
> > > -			error = xfs_attr_rmtval_find_space(attr);
> > > -			if (error)
> > > -				return error;
> > > -		}
> > > -	}
> > > -
> > > -	/*
> > > -	 * Roll through the "value", allocating blocks on disk as
> > > -	 * required.
> > > -	 */
> > > -	if (attr->xattri_blkcnt > 0) {
> > > -		error = xfs_attr_rmtval_set_blk(attr);
> > > -		if (error)
> > > -			return error;
> > > -
> > > -		trace_xfs_das_state_return(attr->xattri_dela_state);
> > > -		return -EAGAIN;
> > > -	}
> > > -
> > > -	error = xfs_attr_rmtval_set_value(args);
> > > -	if (error)
> > > -		return error;
> > > -
> > > -	if (!(args->op_flags & XFS_DA_OP_RENAME)) {
> > > -		/*
> > > -		 * Added a "remote" value, just clear the incomplete flag.
> > > -		 */
> > > -		if (args->rmtblkno > 0)
> > > -			error = xfs_attr3_leaf_clearflag(args);
> > > -
> > > -		return error;
> > > -	}
> > > -
> > > -	/*
> > > -	 * If this is an atomic rename operation, we must "flip" the incomplete
> > > -	 * flags on the "new" and "old" attribute/value pairs so that one
> > > -	 * disappears and one appears atomically.  Then we must remove the "old"
> > > -	 * attribute/value pair.
> > > -	 *
> > > -	 * In a separate transaction, set the incomplete flag on the "old" attr
> > > -	 * and clear the incomplete flag on the "new" attr.
> > > -	 */
> > > -	if (!xfs_hasdelattr(mp)) {
> > > -		error = xfs_attr3_leaf_flipflags(args);
> > > -		if (error)
> > > -			return error;
> > > -		/*
> > > -		 * Commit the flag value change and start the next trans in series.
> > > -		 */
> > > -		attr->xattri_dela_state = XFS_DAS_FLIP_LFLAG;
> > > -		trace_xfs_das_state_return(attr->xattri_dela_state);
> > > -		return -EAGAIN;
> > > -	}
> > > -das_flip_flag:
> > > -	/*
> > > -	 * Dismantle the "old" attribute/value pair by removing a "remote" value
> > > -	 * (if it exists).
> > > -	 */
> > > -	xfs_attr_restore_rmt_blk(args);
> > > -
> > > -	error = xfs_attr_rmtval_invalidate(args);
> > > -	if (error)
> > > -		return error;
> > > -
> > > -	/* Set state in case xfs_attr_rmtval_remove returns -EAGAIN */
> > > -	attr->xattri_dela_state = XFS_DAS_RM_LBLK;
> > > -das_rm_lblk:
> > > -	if (args->rmtblkno) {
> > > -		error = xfs_attr_rmtval_remove(attr);
> > > -		if (error == -EAGAIN)
> > > -			trace_xfs_das_state_return(attr->xattri_dela_state);
> > > -		if (error)
> > > -			return error;
> > > -	}
> > > -
> > > -	/*
> > > -	 * Read in the block containing the "old" attr, then remove the "old"
> > > -	 * attr from that block (neat, huh!)
> > > -	 */
> > > -	error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno,
> > > -				   &bp);
> > > -	if (error)
> > > -		return error;
> > > -
> > > -	xfs_attr3_leaf_remove(bp, args);
> > > -
> > > -	/*
> > > -	 * If the result is small enough, shrink it all into the inode.
> > > -	 */
> > > -	forkoff = xfs_attr_shortform_allfit(bp, dp);
> > > -	if (forkoff)
> > > -		error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
> > > -		/* bp is gone due to xfs_da_shrink_inode */
> > > -
> > > -	return error;
> > > -}
> > > -
> > > -/*
> > >    * Return EEXIST if attr is found, or ENOATTR if not
> > >    */
> > >   STATIC int
> > > @@ -1065,24 +1041,9 @@ xfs_attr_node_addname(
> > >   	struct xfs_da_state_blk		*blk;
> > >   	int				retval = 0;
> > >   	int				error = 0;
> > > -	struct xfs_mount		*mp = args->dp->i_mount;
> > >   	trace_xfs_attr_node_addname(args);
> > > -	/* State machine switch */
> > > -	switch (attr->xattri_dela_state) {
> > > -	case XFS_DAS_FLIP_NFLAG:
> > > -		goto das_flip_flag;
> > > -	case XFS_DAS_FOUND_NBLK:
> > > -		goto das_found_nblk;
> > > -	case XFS_DAS_ALLOC_NODE:
> > > -		goto das_alloc_node;
> > > -	case XFS_DAS_RM_NBLK:
> > > -		goto das_rm_nblk;
> > > -	default:
> > > -		break;
> > > -	}
> > > -
> > >   	/*
> > >   	 * Search to see if name already exists, and get back a pointer
> > >   	 * to where it should go.
> > > @@ -1171,93 +1132,24 @@ xfs_attr_node_addname(
> > >   	attr->xattri_dela_state = XFS_DAS_FOUND_NBLK;
> > >   	trace_xfs_das_state_return(attr->xattri_dela_state);
> > >   	return -EAGAIN;
> > > -das_found_nblk:
> > > -
> > > -	/*
> > > -	 * If there was an out-of-line value, allocate the blocks we
> > > -	 * identified for its storage and copy the value.  This is done
> > > -	 * after we create the attribute so that we don't overflow the
> > > -	 * maximum size of a transaction and/or hit a deadlock.
> > > -	 */
> > > -	if (args->rmtblkno > 0) {
> > > -		/* Open coded xfs_attr_rmtval_set without trans handling */
> > > -		error = xfs_attr_rmtval_find_space(attr);
> > > -		if (error)
> > > -			return error;
> > > -
> > > -		/*
> > > -		 * Roll through the "value", allocating blocks on disk as
> > > -		 * required.  Set the state in case of -EAGAIN return code
> > > -		 */
> > > -		attr->xattri_dela_state = XFS_DAS_ALLOC_NODE;
> > > -das_alloc_node:
> > > -		if (attr->xattri_blkcnt > 0) {
> > > -			error = xfs_attr_rmtval_set_blk(attr);
> > > -			if (error)
> > > -				return error;
> > > -
> > > -			trace_xfs_das_state_return(attr->xattri_dela_state);
> > > -			return -EAGAIN;
> > > -		}
> > > -
> > > -		error = xfs_attr_rmtval_set_value(args);
> > > -		if (error)
> > > -			return error;
> > > -	}
> > > -
> > > -	if (!(args->op_flags & XFS_DA_OP_RENAME)) {
> > > -		/*
> > > -		 * Added a "remote" value, just clear the incomplete flag.
> > > -		 */
> > > -		if (args->rmtblkno > 0)
> > > -			error = xfs_attr3_leaf_clearflag(args);
> > > -		retval = error;
> > > -		goto out;
> > > -	}
> > > -
> > > -	/*
> > > -	 * If this is an atomic rename operation, we must "flip" the incomplete
> > > -	 * flags on the "new" and "old" attribute/value pairs so that one
> > > -	 * disappears and one appears atomically.  Then we must remove the "old"
> > > -	 * attribute/value pair.
> > > -	 *
> > > -	 * In a separate transaction, set the incomplete flag on the "old" attr
> > > -	 * and clear the incomplete flag on the "new" attr.
> > > -	 */
> > > -	if (!xfs_hasdelattr(mp)) {
> > > -		error = xfs_attr3_leaf_flipflags(args);
> > > -		if (error)
> > > -			goto out;
> > > -		/*
> > > -		 * Commit the flag value change and start the next trans in series
> > > -		 */
> > > -		attr->xattri_dela_state = XFS_DAS_FLIP_NFLAG;
> > > -		trace_xfs_das_state_return(attr->xattri_dela_state);
> > > -		return -EAGAIN;
> > > -	}
> > > -das_flip_flag:
> > > -	/*
> > > -	 * Dismantle the "old" attribute/value pair by removing a "remote" value
> > > -	 * (if it exists).
> > > -	 */
> > > -	xfs_attr_restore_rmt_blk(args);
> > > +out:
> > > +	if (state)
> > > +		xfs_da_state_free(state);
> > > -	error = xfs_attr_rmtval_invalidate(args);
> > >   	if (error)
> > >   		return error;
> > > +	return retval;
> > > +}
> > > -	/* Set state in case xfs_attr_rmtval_remove returns -EAGAIN */
> > > -	attr->xattri_dela_state = XFS_DAS_RM_NBLK;
> > > -das_rm_nblk:
> > > -	if (args->rmtblkno) {
> > > -		error = xfs_attr_rmtval_remove(attr);
> > > -
> > > -		if (error == -EAGAIN)
> > > -			trace_xfs_das_state_return(attr->xattri_dela_state);
> > > -
> > > -		if (error)
> > > -			return error;
> > > -	}
> > > +STATIC
> > > +int xfs_attr_node_addname_work(
> > > +	struct xfs_attr_item		*attr)
> > > +{
> > > +	struct xfs_da_args		*args = attr->xattri_da_args;
> > > +	struct xfs_da_state		*state = NULL;
> > > +	struct xfs_da_state_blk		*blk;
> > > +	int				retval = 0;
> > > +	int				error = 0;
> > >   	/*
> > >   	 * Re-find the "old" attribute entry after any split ops. The INCOMPLETE
> > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > > index c80575a..050e5be 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.h
> > > +++ b/fs/xfs/libxfs/xfs_attr.h
> > > @@ -376,8 +376,10 @@ enum xfs_delattr_state {
> > >   	XFS_DAS_UNINIT		= 0,  /* No state has been set yet */
> > >   	XFS_DAS_RM_SHRINK,	      /* We are shrinking the tree */
> > >   	XFS_DAS_FOUND_LBLK,	      /* We found leaf blk for attr */
> > > +	XFS_DAS_ALLOC_LBLK,
> > > +	XFS_DAS_SET_LBLK,
> > >   	XFS_DAS_FOUND_NBLK,	      /* We found node blk for attr */
> > > -	XFS_DAS_FLIP_LFLAG,	      /* Flipped leaf INCOMPLETE attr flag */
> > > +	XFS_DAS_INVAL_LBLK,	      /* Invalidate leaf blks */
> > >   	XFS_DAS_RM_LBLK,	      /* A rename is removing leaf blocks */
> > >   	XFS_DAS_ALLOC_NODE,	      /* We are allocating node blocks */
> > >   	XFS_DAS_FLIP_NFLAG,	      /* Flipped node INCOMPLETE attr flag */
> > > -- 
> > > 2.7.4
> > > 
> > 
>