Re: [PATCH v16 06/11] xfs: Add helper xfs_attr_node_addname_find_attr

[Allison Henderson <allison.henderson@xxxxxxxxxx>]

On 4/1/21 9:05 PM, Chandan Babu R wrote:
> On 26 Mar 2021 at 06:03, Allison Henderson wrote:
> > This patch separates the first half of xfs_attr_node_addname into a
> > helper function xfs_attr_node_addname_find_attr.  It also replaces the
> > restart goto with with an EAGAIN return code driven by a loop in the
> > calling function.  This looks odd now, but will clean up nicly once we
> > introduce the state machine.  It will also enable hoisting the last
> > state out of xfs_attr_node_addname with out having to plumb in a "done"
> > parameter to know if we need to move to the next state or not.
>
> Looks good to me.
>
> Reviewed-by: Chandan Babu R <chandanrlinux@xxxxxxxxx>
Thanks for the reviews!
Allison

> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> > Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> > Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > ---
> >   fs/xfs/libxfs/xfs_attr.c | 86 ++++++++++++++++++++++++++++++------------------
> >   1 file changed, 54 insertions(+), 32 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 531ff56..16159f6 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -52,7 +52,10 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
> >    * Internal routines when attribute list is more than one block.
> >    */
> >   STATIC int xfs_attr_node_get(xfs_da_args_t *args);
> > -STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
> > +STATIC int xfs_attr_node_addname(struct xfs_da_args *args,
> > +				 struct xfs_da_state *state);
> > +STATIC int xfs_attr_node_addname_find_attr(struct xfs_da_args *args,
> > +				 struct xfs_da_state **state);
> >   STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
> >   STATIC int xfs_attr_node_addname_clear_incomplete(struct xfs_da_args *args);
> >   STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
> > @@ -267,6 +270,7 @@ xfs_attr_set_args(
> >   	struct xfs_da_args	*args)
> >   {
> >   	struct xfs_inode	*dp = args->dp;
> > +	struct xfs_da_state     *state;
> >   	int			error;
> >   
> >   	/*
> > @@ -312,7 +316,14 @@ xfs_attr_set_args(
> >   			return error;
> >   	}
> >   
> > -	return xfs_attr_node_addname(args);
> > +	do {
> > +		error = xfs_attr_node_addname_find_attr(args, &state);
> > +		if (error)
> > +			return error;
> > +		error = xfs_attr_node_addname(args, state);
> > +	} while (error == -EAGAIN);
> > +
> > +	return error;
> >   }
> >   
> >   /*
> > @@ -885,47 +896,26 @@ xfs_attr_node_hasname(
> >    * External routines when attribute list size > geo->blksize
> >    *========================================================================*/
> >   
> > -/*
> > - * Add a name to a Btree-format attribute list.
> > - *
> > - * This will involve walking down the Btree, and may involve splitting
> > - * leaf nodes and even splitting intermediate nodes up to and including
> > - * the root node (a special case of an intermediate node).
> > - *
> > - * "Remote" attribute values confuse the issue and atomic rename operations
> > - * add a whole extra layer of confusion on top of that.
> > - */
> >   STATIC int
> > -xfs_attr_node_addname(
> > -	struct xfs_da_args	*args)
> > +xfs_attr_node_addname_find_attr(
> > +	struct xfs_da_args	*args,
> > +	struct xfs_da_state     **state)
> >   {
> > -	struct xfs_da_state	*state;
> > -	struct xfs_da_state_blk	*blk;
> > -	struct xfs_inode	*dp;
> > -	int			retval, error;
> > -
> > -	trace_xfs_attr_node_addname(args);
> > +	int			retval;
> >   
> >   	/*
> > -	 * Fill in bucket of arguments/results/context to carry around.
> > -	 */
> > -	dp = args->dp;
> > -restart:
> > -	/*
> >   	 * Search to see if name already exists, and get back a pointer
> >   	 * to where it should go.
> >   	 */
> > -	retval = xfs_attr_node_hasname(args, &state);
> > +	retval = xfs_attr_node_hasname(args, state);
> >   	if (retval != -ENOATTR && retval != -EEXIST)
> > -		goto out;
> > +		goto error;
> >   
> > -	blk = &state->path.blk[ state->path.active-1 ];
> > -	ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
> >   	if (retval == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
> > -		goto out;
> > +		goto error;
> >   	if (retval == -EEXIST) {
> >   		if (args->attr_flags & XATTR_CREATE)
> > -			goto out;
> > +			goto error;
> >   
> >   		trace_xfs_attr_node_replace(args);
> >   
> > @@ -943,6 +933,38 @@ xfs_attr_node_addname(
> >   		args->rmtvaluelen = 0;
> >   	}
> >   
> > +	return 0;
> > +error:
> > +	if (*state)
> > +		xfs_da_state_free(*state);
> > +	return retval;
> > +}
> > +
> > +/*
> > + * Add a name to a Btree-format attribute list.
> > + *
> > + * This will involve walking down the Btree, and may involve splitting
> > + * leaf nodes and even splitting intermediate nodes up to and including
> > + * the root node (a special case of an intermediate node).
> > + *
> > + * "Remote" attribute values confuse the issue and atomic rename operations
> > + * add a whole extra layer of confusion on top of that.
> > + */
> > +STATIC int
> > +xfs_attr_node_addname(
> > +	struct xfs_da_args	*args,
> > +	struct xfs_da_state	*state)
> > +{
> > +	struct xfs_da_state_blk	*blk;
> > +	struct xfs_inode	*dp;
> > +	int			retval, error;
> > +
> > +	trace_xfs_attr_node_addname(args);
> > +
> > +	dp = args->dp;
> > +	blk = &state->path.blk[state->path.active-1];
> > +	ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
> > +
> >   	retval = xfs_attr3_leaf_add(blk->bp, state->args);
> >   	if (retval == -ENOSPC) {
> >   		if (state->path.active == 1) {
> > @@ -968,7 +990,7 @@ xfs_attr_node_addname(
> >   			if (error)
> >   				goto out;
> >   
> > -			goto restart;
> > +			return -EAGAIN;
> >   		}
> >   
> >   		/*