Re: [PATCH v15 10/22] xfs: Hoist node transaction handling

[Brian Foster <bfoster@redhat.com>]

On Thu, Feb 18, 2021 at 09:53:36AM -0700, Allison Henderson wrote:
> This patch basically hoists the node transaction handling around the
> leaf code we just hoisted.  This will helps setup this area for the
> state machine since the goto is easily replaced with a state since it
> ends with a transaction roll.
> 
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 53 +++++++++++++++++++++++++-----------------------
>  1 file changed, 28 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index bfd4466..56d4b56 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -288,8 +288,34 @@ xfs_attr_set_args(
>  
>  	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>  		error = xfs_attr_leaf_try_add(args, bp);
> -		if (error == -ENOSPC)
> +		if (error == -ENOSPC) {
> +			/*
> +			 * Promote the attribute list to the Btree format.
> +			 */
> +			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.
> +			 */
> +			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;
> +
>  			goto node;
> +		}
>  		else if (error)

		} else if (error) {
			return error;
		}

(I think we usually try to add braces around all branches of an if/else
if at least one branch requires them.)

Otherwise, the factoring looks Ok to me and this does improve on the
wart from the previous patch:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  			return error;
>  
> @@ -381,32 +407,9 @@ xfs_attr_set_args(
>  			/* bp is gone due to xfs_da_shrink_inode */
>  
>  		return error;
> +	}
>  node:
> -		/*
> -		 * Promote the attribute list to the Btree format.
> -		 */
> -		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.
> -		 */
> -		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;
> -	}
>  
>  	do {
>  		error = xfs_attr_node_addname_find_attr(args, &state);
> -- 
> 2.7.4
>