Re: [PATCH v11 05/25] xfs: Split apart xfs_attr_leaf_addname

[Allison Collins <allison.henderson@xxxxxxxxxx>]

On 7/21/20 4:19 PM, Darrick J. Wong wrote:
> On Mon, Jul 20, 2020 at 05:15:46PM -0700, Allison Collins wrote:
> > Split out new helper function xfs_attr_leaf_try_add from
> > xfs_attr_leaf_addname. Because new delayed attribute routines cannot
> > roll transactions, we split off the parts of xfs_attr_leaf_addname that
> > we can use, and move the commit into the calling function.
> >
> > Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx>
> > Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> > Reviewed-by: Chandan Rajendra <chandanrlinux@xxxxxxxxx>
>
> Looks ok,
> Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
Ok, thank you!

Allison
> --D
>
> > ---
> >   fs/xfs/libxfs/xfs_attr.c | 95 +++++++++++++++++++++++++++++++-----------------
> >   1 file changed, 61 insertions(+), 34 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 28a9f8e..6fc6dc6 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -256,10 +256,31 @@ xfs_attr_set_args(
> >   		}
> >   	}
> >   
> > -	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> > +	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> >   		error = xfs_attr_leaf_addname(args);
> > -	else
> > -		error = xfs_attr_node_addname(args);
> > +		if (error != -ENOSPC)
> > +			return error;
> > +
> > +		/*
> > +		 * Finish any deferred work items and roll the transaction once
> > +		 * more.  The goal here is to call node_addname with the inode
> > +		 * and transaction in the same state (inode locked and joined,
> > +		 * transaction clean) no matter how we got to this step.
> > +		 */
> > +		error = xfs_defer_finish(&args->trans);
> > +		if (error)
> > +			return error;
> > +
> > +		/*
> > +		 * Commit the current trans (including the inode) and
> > +		 * start a new one.
> > +		 */
> > +		error = xfs_trans_roll_inode(&args->trans, dp);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> > +	error = xfs_attr_node_addname(args);
> >   	return error;
> >   }
> >   
> > @@ -507,20 +528,21 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
> >    *========================================================================*/
> >   
> >   /*
> > - * Add a name to the leaf attribute list structure
> > + * Tries to add an attribute to an inode in leaf form
> >    *
> > - * 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 function is meant to execute as part of a delayed operation and leaves
> > + * the transaction handling to the caller.  On success the attribute is added
> > + * and the inode and transaction are left dirty.  If there is not enough space,
> > + * the attr data is converted to node format and -ENOSPC is returned. Caller is
> > + * responsible for handling the dirty inode and transaction or adding the attr
> > + * in node format.
> >    */
> >   STATIC int
> > -xfs_attr_leaf_addname(
> > -	struct xfs_da_args	*args)
> > +xfs_attr_leaf_try_add(
> > +	struct xfs_da_args	*args,
> > +	struct xfs_buf		*bp)
> >   {
> > -	struct xfs_buf		*bp;
> > -	int			retval, error, forkoff;
> > -	struct xfs_inode	*dp = args->dp;
> > -
> > -	trace_xfs_attr_leaf_addname(args);
> > +	int			retval, error;
> >   
> >   	/*
> >   	 * Look up the given attribute in the leaf block.  Figure out if
> > @@ -562,31 +584,39 @@ xfs_attr_leaf_addname(
> >   	retval = xfs_attr3_leaf_add(bp, args);
> >   	if (retval == -ENOSPC) {
> >   		/*
> > -		 * Promote the attribute list to the Btree format, then
> > -		 * Commit that transaction so that the node_addname() call
> > -		 * can manage its own transactions.
> > +		 * Promote the attribute list to the Btree format. Unless an
> > +		 * error occurs, retain the -ENOSPC retval
> >   		 */
> >   		error = xfs_attr3_leaf_to_node(args);
> >   		if (error)
> >   			return error;
> > -		error = xfs_defer_finish(&args->trans);
> > -		if (error)
> > -			return error;
> > +	}
> > +	return retval;
> > +out_brelse:
> > +	xfs_trans_brelse(args->trans, bp);
> > +	return retval;
> > +}
> >   
> > -		/*
> > -		 * Commit the current trans (including the inode) and start
> > -		 * a new one.
> > -		 */
> > -		error = xfs_trans_roll_inode(&args->trans, dp);
> > -		if (error)
> > -			return error;
> >   
> > -		/*
> > -		 * Fob the whole rest of the problem off on the Btree code.
> > -		 */
> > -		error = xfs_attr_node_addname(args);
> > +/*
> > + * 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).
> > + */
> > +STATIC int
> > +xfs_attr_leaf_addname(
> > +	struct xfs_da_args	*args)
> > +{
> > +	int			error, forkoff;
> > +	struct xfs_buf		*bp = NULL;
> > +	struct xfs_inode	*dp = args->dp;
> > +
> > +	trace_xfs_attr_leaf_addname(args);
> > +
> > +	error = xfs_attr_leaf_try_add(args, bp);
> > +	if (error)
> >   		return error;
> > -	}
> >   
> >   	/*
> >   	 * Commit the transaction that added the attr name so that
> > @@ -681,9 +711,6 @@ xfs_attr_leaf_addname(
> >   		error = xfs_attr3_leaf_clearflag(args);
> >   	}
> >   	return error;
> > -out_brelse:
> > -	xfs_trans_brelse(args->trans, bp);
> > -	return retval;
> >   }
> >   
> >   /*
> > --
> > 2.7.4