Re: [PATCH v17 10/11] xfs: Add delay ready attr remove routines

[Brian Foster <bfoster@redhat.com>]

On Sat, Apr 24, 2021 at 08:56:45AM -0700, Darrick J. Wong wrote:
> On Fri, Apr 23, 2021 at 08:27:28PM -0700, Allison Henderson wrote:
> > 
> > 
> > On 4/23/21 10:06 AM, Brian Foster wrote:
> > > On Fri, Apr 16, 2021 at 02:20:44AM -0700, Allison Henderson wrote:
> > > > This patch modifies the attr remove routines to be delay ready. This
> > > > means they no longer roll or commit transactions, but instead return
> > > > -EAGAIN to have the calling routine roll and refresh the transaction. In
> > > > this series, xfs_attr_remove_args is merged with
> > > > xfs_attr_node_removename become a new function, xfs_attr_remove_iter.
> > > > This new version uses a sort of state machine like switch to keep track
> > > > of where it was when EAGAIN was returned. A new version of
> > > > xfs_attr_remove_args consists of a simple loop to refresh the
> > > > transaction until the operation is completed. A new XFS_DAC_DEFER_FINISH
> > > > flag is used to finish the transaction where ever the existing code used
> > > > to.
> > > > 
> > > > Calls to xfs_attr_rmtval_remove are replaced with the delay ready
> > > > version __xfs_attr_rmtval_remove. We will rename
> > > > __xfs_attr_rmtval_remove back to xfs_attr_rmtval_remove when we are
> > > > done.
> > > > 
> > > > xfs_attr_rmtval_remove itself is still in use by the set routines (used
> > > > during a rename).  For reasons of preserving existing function, we
> > > > modify xfs_attr_rmtval_remove to call xfs_defer_finish when the flag is
> > > > set.  Similar to how xfs_attr_remove_args does here.  Once we transition
> > > > the set routines to be delay ready, xfs_attr_rmtval_remove is no longer
> > > > used and will be removed.
> > > > 
> > > > This patch also adds a new struct xfs_delattr_context, which we will use
> > > > to keep track of the current state of an attribute operation. The new
> > > > xfs_delattr_state enum is used to track various operations that are in
> > > > progress so that we know not to repeat them, and resume where we left
> > > > off before EAGAIN was returned to cycle out the transaction. Other
> > > > members take the place of local variables that need to retain their
> > > > values across multiple function recalls.  See xfs_attr.h for a more
> > > > detailed diagram of the states.
> > > > 
> > > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > > > ---
> > > >   fs/xfs/libxfs/xfs_attr.c        | 208 +++++++++++++++++++++++++++-------------
> > > >   fs/xfs/libxfs/xfs_attr.h        | 131 +++++++++++++++++++++++++
> > > >   fs/xfs/libxfs/xfs_attr_leaf.c   |   2 +-
> > > >   fs/xfs/libxfs/xfs_attr_remote.c |  48 ++++++----
> > > >   fs/xfs/libxfs/xfs_attr_remote.h |   2 +-
> > > >   fs/xfs/xfs_attr_inactive.c      |   2 +-
> > > >   6 files changed, 305 insertions(+), 88 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > > index ed06b60..0bea8dd 100644
> > > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > ...
> > > > @@ -1231,70 +1262,117 @@ xfs_attr_node_remove_cleanup(
> > > >   }
> > > >   /*
> > > > - * Remove a name from a B-tree attribute list.
> > > > + * Remove the attribute specified in @args.
> > > >    *
> > > >    * This will involve walking down the Btree, and may involve joining
> > > >    * leaf nodes and even joining intermediate nodes up to and including
> > > >    * the root node (a special case of an intermediate node).
> > > > + *
> > > > + * This routine is meant to function as either an in-line or 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_node_removename(
> > > > -	struct xfs_da_args	*args)
> > > > +int
> > > > +xfs_attr_remove_iter(
> > > > +	struct xfs_delattr_context	*dac)
> > > >   {
> > > > -	struct xfs_da_state	*state;
> > > > -	int			retval, error;
> > > > -	struct xfs_inode	*dp = args->dp;
> > > > +	struct xfs_da_args		*args = dac->da_args;
> > > > +	struct xfs_da_state		*state = dac->da_state;
> > > > +	int				retval, error;
> > > > +	struct xfs_inode		*dp = args->dp;
> > > >   	trace_xfs_attr_node_removename(args);
> > > ...
> > > > +	case XFS_DAS_CLNUP:
> > > > +		retval = xfs_attr_node_remove_cleanup(args, state);
> > > 
> > > This is a nit, but when reading the code the "cleanup" name gives the
> > > impression that this is a resource cleanup or something along those
> > > lines, when this is actually a primary component of the operation where
> > > we remove the attr name. That took me a second to find. Could we tweak
> > > the state and rename the helper to something like DAS_RMNAME  /
> > > _node_remove_name() so the naming is a bit more explicit?
> > Sure, this helper is actually added in patch 2 of this set.  I can rename it
> > there?  People have already added their rvb's, but I'm assuming people are
> > not bothered by small tweeks like that?  That way this patch just sort of
> > moves it and XFS_DAS_CLNUP can turn into XFS_DAS_RMNAME here.
> 
> <bikeshed> "RMNAME" looks too similar to "RENAME" for my old eyes, can
> we please pick something else?  Like "RM_NAME", or "REMOVE_NAME" ?
> 

Either of those seem fine to me, FWIW. I think anything that expresses
removal of the name/entry over the more generic "cleanup" name is an
improvement..

Brian

> --D
> 
> > 
> > > 
> > > > -	/*
> > > > -	 * Check to see if the tree needs to be collapsed.
> > > > -	 */
> > > > -	if (retval && (state->path.active > 1)) {
> > > > -		error = xfs_da3_join(state);
> > > > -		if (error)
> > > > -			goto out;
> > > > -		error = xfs_defer_finish(&args->trans);
> > > > -		if (error)
> > > > -			goto out;
> > > >   		/*
> > > > -		 * Commit the Btree join operation and start a new trans.
> > > > +		 * Check to see if the tree needs to be collapsed. Set the flag
> > > > +		 * to indicate that the calling function needs to move the
> > > > +		 * shrink operation
> > > >   		 */
> > > > -		error = xfs_trans_roll_inode(&args->trans, dp);
> > > > -		if (error)
> > > > -			goto out;
> > > > -	}
> > > > +		if (retval && (state->path.active > 1)) {
> > > > +			error = xfs_da3_join(state);
> > > > +			if (error)
> > > > +				goto out;
> > > > -	/*
> > > > -	 * If the result is small enough, push it all into the inode.
> > > > -	 */
> > > > -	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> > > > -		error = xfs_attr_node_shrink(args, state);
> > > > +			dac->flags |= XFS_DAC_DEFER_FINISH;
> > > > +			dac->dela_state = XFS_DAS_RM_SHRINK;
> > > > +			return -EAGAIN;
> > > > +		}
> > > > +
> > > > +		/* fallthrough */
> > > > +	case XFS_DAS_RM_SHRINK:
> > > > +		/*
> > > > +		 * If the result is small enough, push it all into the inode.
> > > > +		 */
> > > > +		if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> > > > +			error = xfs_attr_node_shrink(args, state);
> > > > +
> > > > +		break;
> > > > +	default:
> > > > +		ASSERT(0);
> > > > +		error = -EINVAL;
> > > > +		goto out;
> > > > +	}
> > > > +	if (error == -EAGAIN)
> > > > +		return error;
> > > >   out:
> > > >   	if (state)
> > > >   		xfs_da_state_free(state);
> > > ...
> > > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> > > > index 48d8e9c..908521e7 100644
> > > > --- a/fs/xfs/libxfs/xfs_attr_remote.c
> > > > +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> > > ...
> > > > @@ -685,31 +687,29 @@ xfs_attr_rmtval_remove(
> > > >   	 * Keep de-allocating extents until the remote-value region is gone.
> > > >   	 */
> > > >   	do {
> > > > -		retval = __xfs_attr_rmtval_remove(args);
> > > > -		if (retval && retval != -EAGAIN)
> > > > -			return retval;
> > > > +		error = __xfs_attr_rmtval_remove(&dac);
> > > > +		if (error != -EAGAIN)
> > > > +			break;
> > > 
> > > Shouldn't this retain the (error && error != -EAGAIN) logic to roll the
> > > transaction after the final unmap? Even if this is transient, it's
> > > probably best to preserve behavior if this is unintentional.
> > Sure, I dont think it's intentional, I think back in v10 we had a different
> > arangement here with a helper inside the while() expression that had
> > equivelent error handling logic.  But that got nak'd in the next review and
> > I think I likley forgot to put back this handling.  Will fix.
> > 
> > > 
> > > Otherwise my only remaining feedback was to add/tweak some comments that
> > > I think make the iteration function easier to follow. I've appended a
> > > diff for that. If you agree with the changes feel free to just fold them
> > > in and/or tweak as necessary. With those various nits and Chandan's
> > > feedback addressed, I think this patch looks pretty good.
> > Sure, those all look reasonable.  Will add.  Thanks for the reviews!
> > Allison
> > 
> > > 
> > > Brian
> > > 
> > > --- 8< ---
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > index 0bea8dd34902..ee885c649c26 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > @@ -1289,14 +1289,21 @@ xfs_attr_remove_iter(
> > >   		if (!xfs_inode_hasattr(dp))
> > >   			return -ENOATTR;
> > > +		/*
> > > +		 * Shortform or leaf formats don't require transaction rolls and
> > > +		 * thus state transitions. Call the right helper and return.
> > > +		 */
> > >   		if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
> > >   			ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
> > >   			return xfs_attr_shortform_remove(args);
> > >   		}
> > > -
> > >   		if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> > >   			return xfs_attr_leaf_removename(args);
> > > +		/*
> > > +		 * Node format may require transaction rolls. Set up the
> > > +		 * state context and fall into the state machine.
> > > +		 */
> > >   		if (!dac->da_state) {
> > >   			error = xfs_attr_node_removename_setup(dac);
> > >   			if (error)
> > > @@ -1304,7 +1311,7 @@ xfs_attr_remove_iter(
> > >   			state = dac->da_state;
> > >   		}
> > > -	/* fallthrough */
> > > +		/* fallthrough */
> > >   	case XFS_DAS_RMTBLK:
> > >   		dac->dela_state = XFS_DAS_RMTBLK;
> > > @@ -1316,7 +1323,8 @@ xfs_attr_remove_iter(
> > >   		 */
> > >   		if (args->rmtblkno > 0) {
> > >   			/*
> > > -			 * May return -EAGAIN. Remove blocks until 0 is returned
> > > +			 * May return -EAGAIN. Roll and repeat until all remote
> > > +			 * blocks are removed.
> > >   			 */
> > >   			error = __xfs_attr_rmtval_remove(dac);
> > >   			if (error == -EAGAIN)
> > > @@ -1325,26 +1333,26 @@ xfs_attr_remove_iter(
> > >   				goto out;
> > >   			/*
> > > -			 * Refill the state structure with buffers, the prior
> > > -			 * calls released our buffers.
> > > +			 * Refill the state structure with buffers (the prior
> > > +			 * calls released our buffers) and close out this
> > > +			 * transaction before proceeding.
> > >   			 */
> > >   			ASSERT(args->rmtblkno == 0);
> > >   			error = xfs_attr_refillstate(state);
> > >   			if (error)
> > >   				goto out;
> > > -
> > >   			dac->dela_state = XFS_DAS_CLNUP;
> > >   			dac->flags |= XFS_DAC_DEFER_FINISH;
> > >   			return -EAGAIN;
> > >   		}
> > > +		/* fallthrough */
> > >   	case XFS_DAS_CLNUP:
> > >   		retval = xfs_attr_node_remove_cleanup(args, state);
> > >   		/*
> > > -		 * Check to see if the tree needs to be collapsed. Set the flag
> > > -		 * to indicate that the calling function needs to move the
> > > -		 * shrink operation
> > > +		 * Check to see if the tree needs to be collapsed. If so, roll
> > > +		 * the transacton and fall into the shrink state.
> > >   		 */
> > >   		if (retval && (state->path.active > 1)) {
> > >   			error = xfs_da3_join(state);
> > > @@ -1360,10 +1368,12 @@ xfs_attr_remove_iter(
> > >   	case XFS_DAS_RM_SHRINK:
> > >   		/*
> > >   		 * If the result is small enough, push it all into the inode.
> > > +		 * This is our final state so it's safe to return a dirty
> > > +		 * transaction.
> > >   		 */
> > >   		if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> > >   			error = xfs_attr_node_shrink(args, state);
> > > -
> > > +		ASSERT(error != -EAGAIN);
> > >   		break;
> > >   	default:
> > >   		ASSERT(0);
> > > 
>