On Tue, Aug 20, 2024 at 07:04:52PM +0200, Christoph Hellwig wrote:
> xfs_attr_leaf_try_add is only called by xfs_attr_leaf_addname, and
> merging the two will simplify a following error handling fix.
>
> To facilitate this move the remote block state save/restore helpers up in
> the file so that they don't need forward declarations now.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
Nice cleanup,
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> ---
> fs/xfs/libxfs/xfs_attr.c | 176 ++++++++++++++++-----------------------
> 1 file changed, 74 insertions(+), 102 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index f30bcc64100d56..b9df7a6b1f9d61 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -51,7 +51,6 @@ STATIC int xfs_attr_shortform_addname(xfs_da_args_t *args);
> STATIC int xfs_attr_leaf_get(xfs_da_args_t *args);
> STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args);
> STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
> -STATIC int xfs_attr_leaf_try_add(struct xfs_da_args *args);
>
> /*
> * Internal routines when attribute list is more than one block.
> @@ -437,6 +436,33 @@ xfs_attr_hashval(
> return xfs_attr_hashname(name, namelen);
> }
>
> +/* Save the current remote block info and clear the current pointers. */
> +static void
> +xfs_attr_save_rmt_blk(
> + struct xfs_da_args *args)
> +{
> + args->blkno2 = args->blkno;
> + args->index2 = args->index;
> + args->rmtblkno2 = args->rmtblkno;
> + args->rmtblkcnt2 = args->rmtblkcnt;
> + args->rmtvaluelen2 = args->rmtvaluelen;
> + args->rmtblkno = 0;
> + args->rmtblkcnt = 0;
> + args->rmtvaluelen = 0;
> +}
> +
> +/* Set stored info about a remote block */
> +static void
> +xfs_attr_restore_rmt_blk(
> + struct xfs_da_args *args)
> +{
> + args->blkno = args->blkno2;
> + args->index = args->index2;
> + args->rmtblkno = args->rmtblkno2;
> + args->rmtblkcnt = args->rmtblkcnt2;
> + args->rmtvaluelen = args->rmtvaluelen2;
> +}
> +
> /*
> * PPTR_REPLACE operations require the caller to set the old and new names and
> * values explicitly. Update the canonical fields to the new name and value
> @@ -482,49 +508,77 @@ xfs_attr_complete_op(
> return replace_state;
> }
>
> +/*
> + * Try to add an attribute to an inode in leaf form.
> + */
> static int
> xfs_attr_leaf_addname(
> struct xfs_attr_intent *attr)
> {
> struct xfs_da_args *args = attr->xattri_da_args;
> + struct xfs_buf *bp;
> int error;
>
> ASSERT(xfs_attr_is_leaf(args->dp));
>
> + error = xfs_attr3_leaf_read(args->trans, args->dp, args->owner, 0, &bp);
> + if (error)
> + return error;
> +
> /*
> - * Use the leaf buffer we may already hold locked as a result of
> - * a sf-to-leaf conversion.
> + * Look up the xattr name to set the insertion point for the new xattr.
> */
> - error = xfs_attr_leaf_try_add(args);
> -
> - if (error == -ENOSPC) {
> - error = xfs_attr3_leaf_to_node(args);
> - if (error)
> - return error;
> + error = xfs_attr3_leaf_lookup_int(bp, args);
> + switch (error) {
> + case -ENOATTR:
> + if (args->op_flags & XFS_DA_OP_REPLACE)
> + goto out_brelse;
> + break;
> + case -EEXIST:
> + if (!(args->op_flags & XFS_DA_OP_REPLACE))
> + goto out_brelse;
>
> + trace_xfs_attr_leaf_replace(args);
> /*
> - * We're not in leaf format anymore, so roll the transaction and
> - * retry the add to the newly allocated node block.
> + * Save the existing remote attr state so that the current
> + * values reflect the state of the new attribute we are about to
> + * add, not the attribute we just found and will remove later.
> */
> - attr->xattri_dela_state = XFS_DAS_NODE_ADD;
> - goto out;
> + xfs_attr_save_rmt_blk(args);
> + break;
> + case 0:
> + break;
> + default:
> + goto out_brelse;
> }
> - if (error)
> - return error;
>
> /*
> * We need to commit and roll if we need to allocate remote xattr blocks
> * or perform more xattr manipulations. Otherwise there is nothing more
> * to do and we can return success.
> */
> - if (args->rmtblkno)
> + error = xfs_attr3_leaf_add(bp, args);
> + if (error) {
> + if (error != -ENOSPC)
> + return error;
> + error = xfs_attr3_leaf_to_node(args);
> + if (error)
> + return error;
> +
> + attr->xattri_dela_state = XFS_DAS_NODE_ADD;
> + } else if (args->rmtblkno) {
> attr->xattri_dela_state = XFS_DAS_LEAF_SET_RMT;
> - else
> - attr->xattri_dela_state = xfs_attr_complete_op(attr,
> - XFS_DAS_LEAF_REPLACE);
> -out:
> + } else {
> + attr->xattri_dela_state =
> + xfs_attr_complete_op(attr, XFS_DAS_LEAF_REPLACE);
> + }
> +
> trace_xfs_attr_leaf_addname_return(attr->xattri_dela_state, args->dp);
> return error;
> +
> +out_brelse:
> + xfs_trans_brelse(args->trans, bp);
> + return error;
> }
>
> /*
> @@ -1170,88 +1224,6 @@ xfs_attr_shortform_addname(
> * External routines when attribute list is one block
> *========================================================================*/
>
> -/* Save the current remote block info and clear the current pointers. */
> -static void
> -xfs_attr_save_rmt_blk(
> - struct xfs_da_args *args)
> -{
> - args->blkno2 = args->blkno;
> - args->index2 = args->index;
> - args->rmtblkno2 = args->rmtblkno;
> - args->rmtblkcnt2 = args->rmtblkcnt;
> - args->rmtvaluelen2 = args->rmtvaluelen;
> - args->rmtblkno = 0;
> - args->rmtblkcnt = 0;
> - args->rmtvaluelen = 0;
> -}
> -
> -/* Set stored info about a remote block */
> -static void
> -xfs_attr_restore_rmt_blk(
> - struct xfs_da_args *args)
> -{
> - args->blkno = args->blkno2;
> - args->index = args->index2;
> - args->rmtblkno = args->rmtblkno2;
> - args->rmtblkcnt = args->rmtblkcnt2;
> - args->rmtvaluelen = args->rmtvaluelen2;
> -}
> -
> -/*
> - * Tries to add an attribute to an inode in leaf form
> - *
> - * This function is meant to execute as part of a delayed operation and leaves
> - * the transaction handling to the caller. On success the attribute is added
> - * and the inode and transaction are left dirty. If there is not enough space,
> - * the attr data is converted to node format and -ENOSPC is returned. Caller is
> - * responsible for handling the dirty inode and transaction or adding the attr
> - * in node format.
> - */
> -STATIC int
> -xfs_attr_leaf_try_add(
> - struct xfs_da_args *args)
> -{
> - struct xfs_buf *bp;
> - int error;
> -
> - error = xfs_attr3_leaf_read(args->trans, args->dp, args->owner, 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);
> - switch (error) {
> - case -ENOATTR:
> - if (args->op_flags & XFS_DA_OP_REPLACE)
> - goto out_brelse;
> - break;
> - case -EEXIST:
> - if (!(args->op_flags & XFS_DA_OP_REPLACE))
> - goto out_brelse;
> -
> - trace_xfs_attr_leaf_replace(args);
> - /*
> - * Save the existing remote attr state so that the current
> - * values reflect the state of the new attribute we are about to
> - * add, not the attribute we just found and will remove later.
> - */
> - xfs_attr_save_rmt_blk(args);
> - break;
> - case 0:
> - break;
> - default:
> - goto out_brelse;
> - }
> -
> - return xfs_attr3_leaf_add(bp, args);
> -
> -out_brelse:
> - xfs_trans_brelse(args->trans, bp);
> - return error;
> -}
> -
> /*
> * Return EEXIST if attr is found, or ENOATTR if not
> */
> --
> 2.43.0
>
>
