Re: [PATCH v4 08/17] xfs: Factor out xfs_attr_leaf_addname helper

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Nov 06, 2019 at 06:27:52PM -0700, Allison Collins wrote:
> Factor out new helper function xfs_attr_leaf_try_add.
> Because new delayed attribute routines cannot roll
> transactions, we carve off the parts of
> xfs_attr_leaf_addname that we can use.  This will help
> to reduce repetitive code later when we introduce
> delayed attributes.
> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 84 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 51 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 212995f..dda2eba 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -305,10 +305,33 @@ 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
> +		if (error == -ENOSPC) {
> +			/*
> +			 * Commit that transaction so that the node_addname()
> +			 * call can manage its own transactions.
> +			 */
> +			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;
> +
> +			/*
> +			 * Fob the rest of the problem off on the Btree code.
> +			 */
> +			error = xfs_attr_node_addname(args);
> +		}
> +	} else {
>  		error = xfs_attr_node_addname(args);
> +	}
>  	return error;


>  }
>  
> @@ -601,21 +624,12 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
>   * External routines when attribute list is one block
>   *========================================================================*/
>  
> -/*
> - * 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)
> +xfs_attr_leaf_try_add(

(total stream of consciousness here...)

AFAICT the old _addname function's responsibilities were:

1 Try to add a new attr key entry to the leaf block, with INCOMPLETE set
  if it's a rename op or we need to set a remote value.
2 If there wasn't space in the leaf block, convert to node format, call
  the node version of this function, and exit.
3 Allocating blocks for the remote attr value and writing them, if
  applicable
4 If it's a rename operation, clearing the INCOMPLETE flag on the new
  entry; setting it on the old entry; and then removing the old entry.
5 Clearing the INCOMPLETE flag on the new entry when we're done writing
  a remote value (if applicable)

I think we arrive at this split so that we don't have a transaction roll
in the middle of the function, right?  And also to make the "convert to
node format and roll" bits go elsewhere?

The way I'm thinking about how to accomplish this is...

xfs_attr_leaf_addname should be renamed xfs_attr_leaf_setname, and then
hoist (1) into a separate function, move (2) into xfs_attr_set_args, and
hoist (4) into a separate function.

...ok, so let's test how closely my understanding fits the changes made
in this patch:

_try_add is basically (1).

Most of (2) happened, though the call to xfs_attr3_leaf_to_node ought to
go into the caller so that the conversion stays with the defer_finish
and roll.

(4) could still be done, maybe as a separate prep patch.

Hm, ok, I think I understand what this patch does.  The call site in
xfs_attr_set_args would be clearer (and less indenty) if it looked like:

	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
		error = xfs_attr_leaf_addname(args);
		if (error == 0 || error != -ENOSPC)
			return error;

		/* Promote the attribute list to node format. */
		error = xfs_attr3_leaf_to_node(args);
		if (error)
			return error;

		/*
		 * Commit that transaction so that the node_addname()
		 * call can manage its own transactions.
		 */
		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;
	}

	return xfs_attr_node_addname(args);

But otherwise it looks decent, assuming I understood any of it. :)

--D

> +	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
> @@ -661,31 +675,35 @@ 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;
> +}
>  
> -		/*
> -		 * 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
> -- 
> 2.7.4
>