Re: [PATCH v4 16/17] xfs: Add delay ready attr remove routines

[Allison Collins <allison.henderson@xxxxxxxxxx>]

On 11/8/19 2:37 PM, Darrick J. Wong wrote:
> On Wed, Nov 06, 2019 at 06:28:00PM -0700, Allison Collins 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 has become
> > xfs_attr_remove_later, which uses a state machine to keep track
> > of where it was when EAGAIN was returned.  xfs_attr_node_removename
> > has also been modified to use the state machine, and a  new version of
> > xfs_attr_remove_args consists of a simple loop to refresh the
> > transaction until the operation is completed.
> >
> > Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx>
> > ---
> >   fs/xfs/libxfs/xfs_attr.c | 123 +++++++++++++++++++++++++++++++++++++++--------
> >   fs/xfs/libxfs/xfs_attr.h |   1 +
> >   2 files changed, 104 insertions(+), 20 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 626d4a98..38d5c5c 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -369,10 +369,56 @@ xfs_has_attr(
> >    */
> >   int
> >   xfs_attr_remove_args(
> > +	struct xfs_da_args	*args)
> > +{
> > +	int			error = 0;
> > +	int			err2 = 0;
> > +
> > +	do {
> > +		error = xfs_attr_remove_later(args);
> > +		if (error && error != -EAGAIN)
> > +			goto out;
> > +
> > +		xfs_trans_log_inode(args->trans, args->dp,
> > +			XFS_ILOG_CORE | XFS_ILOG_ADATA);
>
> Don't the individual pieces of attr removal log ADATA on their own, when
> needed?
>
> If it weren't for that, this could just be xfs_trans_roll_inode, right?

Oh i see.  I will see if I can simplify that.  Thanks!
Allison

> --D
>
> > +
> > +		err2 = xfs_trans_roll(&args->trans);
> > +		if (err2) {
> > +			error = err2;
> > +			goto out;
> > +		}
> > +
> > +		/* Rejoin inode */
> > +		xfs_trans_ijoin(args->trans, args->dp, 0);
> > +
> > +	} while (error == -EAGAIN);
> > +out:
> > +	return error;
> > +}
> > +
> > +/*
> > + * Remove the attribute specified in @args.
> > + * This routine is meant to function as a delayed operation, and may return
> > + * -EGAIN 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.
> > + */
> > +int
> > +xfs_attr_remove_later(
> >   	struct xfs_da_args      *args)
> >   {
> >   	struct xfs_inode	*dp = args->dp;
> > -	int			error;
> > +	int			error = 0;
> > +
> > +	/* State machine switch */
> > +	switch (args->dc.dc_state) {
> > +	case XFS_DC_RM_INVALIDATE:
> > +	case XFS_DC_RM_SHRINK:
> > +	case XFS_DC_RM_NODE_BLKS:
> > +		goto node;
> > +	default:
> > +		break;
> > +	}
> >   
> >   	if (!xfs_inode_hasattr(dp)) {
> >   		error = -ENOATTR;
> > @@ -382,6 +428,7 @@ xfs_attr_remove_args(
> >   	} else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> >   		error = xfs_attr_leaf_removename(args);
> >   	} else {
> > +node:
> >   		error = xfs_attr_node_removename(args);
> >   	}
> >   
> > @@ -892,9 +939,6 @@ xfs_attr_leaf_removename(
> >   		/* bp is gone due to xfs_da_shrink_inode */
> >   		if (error)
> >   			return error;
> > -		error = xfs_defer_finish(&args->trans);
> > -		if (error)
> > -			return error;
> >   	}
> >   	return 0;
> >   }
> > @@ -1212,6 +1256,11 @@ xfs_attr_node_addname(
> >    * 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 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_node_removename(
> > @@ -1222,12 +1271,29 @@ xfs_attr_node_removename(
> >   	struct xfs_buf		*bp;
> >   	int			retval, error, forkoff;
> >   	struct xfs_inode	*dp = args->dp;
> > +	int			done = 0;
> >   
> >   	trace_xfs_attr_node_removename(args);
> > +	state = args->dc.da_state;
> > +	blk = args->dc.blk;
> > +
> > +	/* State machine switch */
> > +	switch (args->dc.dc_state) {
> > +	case XFS_DC_RM_NODE_BLKS:
> > +		goto rm_node_blks;
> > +	case XFS_DC_RM_INVALIDATE:
> > +		goto rm_invalidate;
> > +	case XFS_DC_RM_SHRINK:
> > +		goto rm_shrink;
> > +	default:
> > +		break;
> > +	}
> >   
> >   	error = xfs_attr_node_hasname(args, &state);
> >   	if (error != -EEXIST)
> >   		goto out;
> > +	else
> > +		error = 0;
> >   
> >   	/*
> >   	 * If there is an out-of-line value, de-allocate the blocks.
> > @@ -1237,6 +1303,14 @@ xfs_attr_node_removename(
> >   	blk = &state->path.blk[ state->path.active-1 ];
> >   	ASSERT(blk->bp != NULL);
> >   	ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
> > +
> > +	/*
> > +	 * Store blk and state in the context incase we need to cycle out the
> > +	 * transaction
> > +	 */
> > +	args->dc.blk = blk;
> > +	args->dc.da_state = state;
> > +
> >   	if (args->rmtblkno > 0) {
> >   		/*
> >   		 * Fill in disk block numbers in the state structure
> > @@ -1255,13 +1329,30 @@ xfs_attr_node_removename(
> >   		if (error)
> >   			goto out;
> >   
> > -		error = xfs_trans_roll_inode(&args->trans, args->dp);
> > +		args->dc.dc_state = XFS_DC_RM_INVALIDATE;
> > +		return -EAGAIN;
> > +rm_invalidate:
> > +		error = xfs_attr_rmtval_invalidate(args);
> >   		if (error)
> >   			goto out;
> > +rm_node_blks:
> > +		/*
> > +		 * Unmap value blocks for this attr.  This is similar to
> > +		 * xfs_attr_rmtval_remove, but open coded here to return EAGAIN
> > +		 * for new transactions
> > +		 */
> > +		while (!done && !error) {
> > +			error = xfs_bunmapi(args->trans, args->dp,
> > +				    args->rmtblkno, args->rmtblkcnt,
> > +				    XFS_BMAPI_ATTRFORK, 1, &done);
> > +			if (error)
> > +				return error;
> >   
> > -		error = xfs_attr_rmtval_remove(args);
> > -		if (error)
> > -			goto out;
> > +			if (!done) {
> > +				args->dc.dc_state = XFS_DC_RM_NODE_BLKS;
> > +				return -EAGAIN;
> > +			}
> > +		}
> >   
> >   		/*
> >   		 * Refill the state structure with buffers, the prior calls
> > @@ -1287,17 +1378,12 @@ xfs_attr_node_removename(
> >   		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.
> > -		 */
> > -		error = xfs_trans_roll_inode(&args->trans, dp);
> > -		if (error)
> > -			goto out;
> > +
> > +		args->dc.dc_state = XFS_DC_RM_SHRINK;
> > +		return -EAGAIN;
> >   	}
> >   
> > +rm_shrink:
> >   	/*
> >   	 * If the result is small enough, push it all into the inode.
> >   	 */
> > @@ -1319,9 +1405,6 @@ xfs_attr_node_removename(
> >   			/* bp is gone due to xfs_da_shrink_inode */
> >   			if (error)
> >   				goto out;
> > -			error = xfs_defer_finish(&args->trans);
> > -			if (error)
> > -				goto out;
> >   		} else
> >   			xfs_trans_brelse(args->trans, bp);
> >   	}
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index 3b5dad4..fb8bf5b 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -152,6 +152,7 @@ int xfs_attr_set_args(struct xfs_da_args *args);
> >   int xfs_attr_remove(struct xfs_inode *dp, struct xfs_name *name, int flags);
> >   int xfs_has_attr(struct xfs_da_args *args);
> >   int xfs_attr_remove_args(struct xfs_da_args *args);
> > +int xfs_attr_remove_later(struct xfs_da_args *args);
> >   int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
> >   		  int flags, struct attrlist_cursor_kern *cursor);
> >   bool xfs_attr_namecheck(const void *name, size_t length);
> > --
> > 2.7.4