On Mon, May 09, 2022 at 10:41:23AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Reviewed-by: Allison Henderson<allison.henderson@xxxxxxxxxx>
Took me a while to figure this out, but looks fine to me.
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--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
>
