Re: [PATCH v6 12/16] xfs: Add helper function xfs_attr_init_unmapstate

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

On Sat, Jan 18, 2020 at 03:50:31PM -0700, Allison Collins wrote:
> This patch helps to pre-simplify xfs_attr_node_removename by modularizing
> the code around the transactions into helper functions.  This will make
> the function easier to follow when we introduce delayed attributes.
> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 45 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index e9d22c1..453ea59 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -1202,6 +1202,36 @@ xfs_attr_node_addname(
>  }
>  
>  /*
> + * Set up the state for unmap and set the incomplete flag

"Mark an attribute entry INCOMPLETE and save pointers to the relevant
buffers for later deletion of the entry." ?

> + */
> +STATIC int
> +xfs_attr_init_unmapstate(

I'm hung up on this name -- it marks the entry incomplete and saves some
breadcrumbs for erasing data later, which I guess is "unmap state"?
What do you think of:

xfs_attr_leaf_mark_incomplete()

> +	struct xfs_da_args	*args,
> +	struct xfs_da_state	*state)
> +{
> +	int error;
> +
> +	/*
> +	 * Fill in disk block numbers in the state structure
> +	 * so that we can get the buffers back after we commit
> +	 * several transactions in the following calls.
> +	 */
> +	error = xfs_attr_fillstate(state);
> +	if (error)
> +		return error;
> +
> +	/*
> +	 * Mark the attribute as INCOMPLETE

Has this stopped doing the bunmapi call, or was the comment inaccurate?
I'm guessing the second if it's necessary to save state to recover
buffer pointers later...?

--D

> +	 */
> +	error = xfs_attr3_leaf_setflag(args);
> +	if (error)
> +		return error;
> +
> +	return 0;
> +}
> +
> +
> +/*
>   * Remove a name from a B-tree attribute list.
>   *
>   * This will involve walking down the Btree, and may involve joining
> @@ -1233,20 +1263,7 @@ xfs_attr_node_removename(
>  	ASSERT(blk->bp != NULL);
>  	ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
>  	if (args->rmtblkno > 0) {
> -		/*
> -		 * Fill in disk block numbers in the state structure
> -		 * so that we can get the buffers back after we commit
> -		 * several transactions in the following calls.
> -		 */
> -		error = xfs_attr_fillstate(state);
> -		if (error)
> -			goto out;
> -
> -		/*
> -		 * Mark the attribute as INCOMPLETE, then bunmapi() the
> -		 * remote value.
> -		 */
> -		error = xfs_attr3_leaf_setflag(args);
> +		error = xfs_attr_init_unmapstate(args, state);
>  		if (error)
>  			goto out;
>  
> -- 
> 2.7.4
>