Re: [PATCH v24 11/11] xfs: Add helper function xfs_attr_leaf_addname

[Allison Henderson <allison.henderson@xxxxxxxxxx>]

On 8/30/21 7:17 AM, Chandan Babu R wrote:
> On 25 Aug 2021 at 04:14, Allison Henderson wrote:
> > This patch adds a helper function xfs_attr_leaf_addname.  While this
> > does help to break down xfs_attr_set_iter, it does also hoist out some
> > of the state management.  This patch has been moved to the end of the
> > clean up series for further discussion.
>
> Looks good to me.
>
> Reviewed-by: Chandan Babu R <chandanrlinux@xxxxxxxxx>

Great, thankyou!

Allison

> > Suggested-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> > Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > ---
> >   fs/xfs/libxfs/xfs_attr.c | 110 +++++++++++++++++++++------------------
> >   fs/xfs/xfs_trace.h       |   1 +
> >   2 files changed, 61 insertions(+), 50 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index c3fdf232cd51..7150f0e051a0 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -284,6 +284,65 @@ xfs_attr_sf_addname(
> >   	return -EAGAIN;
> >   }
> >   
> > +STATIC int
> > +xfs_attr_leaf_addname(
> > +	struct xfs_attr_item	*attr)
> > +{
> > +	struct xfs_da_args	*args = attr->xattri_da_args;
> > +	struct xfs_inode	*dp = args->dp;
> > +	int			error;
> > +
> > +	if (xfs_attr_is_leaf(dp)) {
> > +		error = xfs_attr_leaf_try_add(args, attr->xattri_leaf_bp);
> > +		if (error == -ENOSPC) {
> > +			error = xfs_attr3_leaf_to_node(args);
> > +			if (error)
> > +				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.
> > +			 *
> > +			 * At this point, we are still in XFS_DAS_UNINIT, but
> > +			 * when we come back, we'll be a node, so we'll fall
> > +			 * down into the node handling code below
> > +			 */
> > +			trace_xfs_attr_set_iter_return(
> > +				attr->xattri_dela_state, args->dp);
> > +			return -EAGAIN;
> > +		}
> > +
> > +		if (error)
> > +			return error;
> > +
> > +		attr->xattri_dela_state = XFS_DAS_FOUND_LBLK;
> > +	} else {
> > +		error = xfs_attr_node_addname_find_attr(attr);
> > +		if (error)
> > +			return error;
> > +
> > +		error = xfs_attr_node_addname(attr);
> > +		if (error)
> > +			return error;
> > +
> > +		/*
> > +		 * If addname was successful, and we dont need to alloc or
> > +		 * remove anymore blks, we're done.
> > +		 */
> > +		if (!args->rmtblkno &&
> > +		    !(args->op_flags & XFS_DA_OP_RENAME))
> > +			return 0;
> > +
> > +		attr->xattri_dela_state = XFS_DAS_FOUND_NBLK;
> > +	}
> > +
> > +	trace_xfs_attr_leaf_addname_return(attr->xattri_dela_state, args->dp);
> > +	return -EAGAIN;
> > +}
> > +
> >   /*
> >    * Set the attribute specified in @args.
> >    * This routine is meant to function as a delayed operation, and may return
> > @@ -319,57 +378,8 @@ xfs_attr_set_iter(
> >   			attr->xattri_leaf_bp = NULL;
> >   		}
> >   
> > -		if (xfs_attr_is_leaf(dp)) {
> > -			error = xfs_attr_leaf_try_add(args,
> > -						      attr->xattri_leaf_bp);
> > -			if (error == -ENOSPC) {
> > -				error = xfs_attr3_leaf_to_node(args);
> > -				if (error)
> > -					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.
> > -				 *
> > -				 * At this point, we are still in
> > -				 * XFS_DAS_UNINIT, but when we come back, we'll
> > -				 * be a node, so we'll fall down into the node
> > -				 * handling code below
> > -				 */
> > -				trace_xfs_attr_set_iter_return(
> > -					attr->xattri_dela_state, args->dp);
> > -				return -EAGAIN;
> > -			} else if (error) {
> > -				return error;
> > -			}
> > -
> > -			attr->xattri_dela_state = XFS_DAS_FOUND_LBLK;
> > -		} else {
> > -			error = xfs_attr_node_addname_find_attr(attr);
> > -			if (error)
> > -				return error;
> > +		return xfs_attr_leaf_addname(attr);
> >   
> > -			error = xfs_attr_node_addname(attr);
> > -			if (error)
> > -				return error;
> > -
> > -			/*
> > -			 * If addname was successful, and we dont need to alloc
> > -			 * or remove anymore blks, we're done.
> > -			 */
> > -			if (!args->rmtblkno &&
> > -			    !(args->op_flags & XFS_DA_OP_RENAME))
> > -				return 0;
> > -
> > -			attr->xattri_dela_state = XFS_DAS_FOUND_NBLK;
> > -		}
> > -		trace_xfs_attr_set_iter_return(attr->xattri_dela_state,
> > -					       args->dp);
> > -		return -EAGAIN;
> >   	case XFS_DAS_FOUND_LBLK:
> >   		/*
> >   		 * If there was an out-of-line value, allocate the blocks we
> > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> > index 1033a95fbf8e..77a78b5b1a29 100644
> > --- a/fs/xfs/xfs_trace.h
> > +++ b/fs/xfs/xfs_trace.h
> > @@ -4132,6 +4132,7 @@ DEFINE_EVENT(xfs_das_state_class, name, \
> >   	TP_ARGS(das, ip))
> >   DEFINE_DAS_STATE_EVENT(xfs_attr_sf_addname_return);
> >   DEFINE_DAS_STATE_EVENT(xfs_attr_set_iter_return);
> > +DEFINE_DAS_STATE_EVENT(xfs_attr_leaf_addname_return);
> >   DEFINE_DAS_STATE_EVENT(xfs_attr_node_addname_return);
> >   DEFINE_DAS_STATE_EVENT(xfs_attr_remove_iter_return);
> >   DEFINE_DAS_STATE_EVENT(xfs_attr_rmtval_remove_return);