Re: [PATCH v8 11/20] xfs: Add helper function xfs_attr_node_shrink

[Allison Collins <allison.henderson@xxxxxxxxxx>]

On 4/15/20 3:16 AM, Chandan Rajendra wrote:
> On Saturday, April 4, 2020 3:42 AM Allison Collins wrote:
> > This patch adds a new helper function xfs_attr_node_shrink used to
> > shrink an attr name into an inode if it is small enough.  This helps to
> > modularize the greater calling function xfs_attr_node_removename.
> >
> > Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx>
> > ---
> >   fs/xfs/libxfs/xfs_attr.c | 67 ++++++++++++++++++++++++++++++------------------
> >   1 file changed, 42 insertions(+), 25 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index db5a99c..27a9bb5 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -1103,6 +1103,45 @@ xfs_attr_node_addname(
> >   }
> >   
> >   /*
> > + * Shrink an attribute from leaf to shortform
> > + */
> > +STATIC int
> > +xfs_attr_node_shrink(
> > +	struct xfs_da_args	*args,
> > +	struct xfs_da_state     *state)
> > +{
> > +	struct xfs_inode	*dp = args->dp;
> > +	int			error, forkoff;
> > +	struct xfs_buf		*bp;
> > +
> > +	/*
> > +	 * Have to get rid of the copy of this dabuf in the state.
> > +	 */
> > +	ASSERT(state->path.active == 1);
> > +	ASSERT(state->path.blk[0].bp);
> > +	state->path.blk[0].bp = NULL;
> > +
> > +	error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp);
> > +	if (error)
> > +		return error;
> > +
> > +	forkoff = xfs_attr_shortform_allfit(bp, dp);
> > +	if (forkoff) {
> > +		error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
> > +		/* bp is gone due to xfs_da_shrink_inode */
> > +		if (error)
> > +			return error;
> > +
> > +		error = xfs_defer_finish(&args->trans);
> > +		if (error)
> > +			return error;
> > +	} else
> > +		xfs_trans_brelse(args->trans, bp);
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> >    * Remove a name from a B-tree attribute list.
> >    *
> >    * This will involve walking down the Btree, and may involve joining
> > @@ -1115,8 +1154,7 @@ xfs_attr_node_removename(
> >   {
> >   	struct xfs_da_state	*state;
> >   	struct xfs_da_state_blk	*blk;
> > -	struct xfs_buf		*bp;
> > -	int			retval, error, forkoff;
> > +	int			retval, error;
> >   	struct xfs_inode	*dp = args->dp;
> >   
> >   	trace_xfs_attr_node_removename(args);
> > @@ -1197,31 +1235,10 @@ xfs_attr_node_removename(
> >   	/*
> >   	 * If the result is small enough, push it all into the inode.
> >   	 */
> > -	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> > -		/*
> > -		 * Have to get rid of the copy of this dabuf in the state.
> > -		 */
> > -		ASSERT(state->path.active == 1);
> > -		ASSERT(state->path.blk[0].bp);
> > -		state->path.blk[0].bp = NULL;
> > -
> > -		error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp);
> > -		if (error)
> > -			goto out;
> > +	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> > +		error = xfs_attr_node_shrink(args, state);
>
> If a non-zero error value is returned by the above statement, the following
> statement i.e. "error = 0" will overwrite it.
> Apart from that everything else looks fine.
>
> Reviewed-by: Chandan Rajendra <chandanrlinux@xxxxxxxxx>

Yep, I've removed the trailing error = 0; in the new v9.  Thanks for the 
review!

Allison
> >   
> > -		if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) {
> > -			error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
> > -			/* 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);
> > -	}
> >   	error = 0;
> > -
> >   out:
> >   	if (state)
> >   		xfs_da_state_free(state);