Re: [PATCH 03/18] xfs: make xattri_leaf_bp more useful

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, May 09, 2022 at 10:41:23AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> We currently set it and hold it when converting from short to leaf
> form, then release it only to immediately look it back up again
> to do the leaf insert.
> 
> Do a bit of refactoring to xfs_attr_leaf_try_add() to avoid this
> messy handling of the newly allocated leaf buffer.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Allison Henderson<allison.henderson@oracle.com>

Took me a while to figure this out, but looks fine to me.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_attr.c | 50 +++++++++++++++++++++++++---------------
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index b3d918195160..a4b0b20a3bab 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -319,7 +319,15 @@ xfs_attr_leaf_addname(
>  	int			error;
>  
>  	if (xfs_attr_is_leaf(dp)) {
> +
> +		/*
> +		 * Use the leaf buffer we may already hold locked as a result of
> +		 * a sf-to-leaf conversion. The held buffer is no longer valid
> +		 * after this call, regardless of the result.
> +		 */
>  		error = xfs_attr_leaf_try_add(args, attr->xattri_leaf_bp);
> +		attr->xattri_leaf_bp = NULL;
> +
>  		if (error == -ENOSPC) {
>  			error = xfs_attr3_leaf_to_node(args);
>  			if (error)
> @@ -341,6 +349,8 @@ xfs_attr_leaf_addname(
>  		}
>  		next_state = XFS_DAS_FOUND_LBLK;
>  	} else {
> +		ASSERT(!attr->xattri_leaf_bp);
> +
>  		error = xfs_attr_node_addname_find_attr(attr);
>  		if (error)
>  			return error;
> @@ -396,12 +406,6 @@ xfs_attr_set_iter(
>  		 */
>  		if (xfs_attr_is_shortform(dp))
>  			return xfs_attr_sf_addname(attr);
> -		if (attr->xattri_leaf_bp != NULL) {
> -			xfs_trans_bhold_release(args->trans,
> -						attr->xattri_leaf_bp);
> -			attr->xattri_leaf_bp = NULL;
> -		}
> -
>  		return xfs_attr_leaf_addname(attr);
>  
>  	case XFS_DAS_FOUND_LBLK:
> @@ -992,18 +996,31 @@ xfs_attr_leaf_try_add(
>  	struct xfs_da_args	*args,
>  	struct xfs_buf		*bp)
>  {
> -	int			retval;
> +	int			error;
>  
>  	/*
> -	 * Look up the given attribute in the leaf block.  Figure out if
> -	 * the given flags produce an error or call for an atomic rename.
> +	 * If the caller provided a buffer to us, it is locked and held in
> +	 * the transaction because it just did a shortform to leaf conversion.
> +	 * Hence we don't need to read it again. Otherwise read in the leaf
> +	 * buffer.
>  	 */
> -	retval = xfs_attr_leaf_hasname(args, &bp);
> -	if (retval != -ENOATTR && retval != -EEXIST)
> -		return retval;
> -	if (retval == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
> +	if (bp) {
> +		xfs_trans_bhold_release(args->trans, bp);
> +	} else {
> +		error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp);
> +		if (error)
> +			return error;
> +	}
> +
> +	/*
> +	 * Look up the xattr name to set the insertion point for the new xattr.
> +	 */
> +	error = xfs_attr3_leaf_lookup_int(bp, args);
> +	if (error != -ENOATTR && error != -EEXIST)
>  		goto out_brelse;
> -	if (retval == -EEXIST) {
> +	if (error == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
> +		goto out_brelse;
> +	if (error == -EEXIST) {
>  		if (args->attr_flags & XATTR_CREATE)
>  			goto out_brelse;
>  
> @@ -1023,14 +1040,11 @@ xfs_attr_leaf_try_add(
>  		args->rmtvaluelen = 0;
>  	}
>  
> -	/*
> -	 * Add the attribute to the leaf block
> -	 */
>  	return xfs_attr3_leaf_add(bp, args);
>  
>  out_brelse:
>  	xfs_trans_brelse(args->trans, bp);
> -	return retval;
> +	return error;
>  }
>  
>  /*
> -- 
> 2.35.1
>