On Fri, 2022-05-06 at 19:45 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Now that xfs_attri_set_iter() has initial states for removing
> attributes, switch the pure attribute removal code over to using it.
> This requires attrs being removed to always be marked as INCOMPLETE
> before we start the removal due to the fact we look up the attr to
> remove again in xfs_attr_node_remove_attr().
>
> Note: this drops the fillstate/refillstate optimisations from
> the remove path that avoid having to look up the path again after
> setting the incomplete flag and removeing remote attrs. Restoring
> that optimisation to this path is future Dave's problem.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
Ok, I think this looks ok now
Reviewed-by: Allison Henderson<allison.henderson@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_attr.c | 21 +++++++++------------
> fs/xfs/libxfs/xfs_attr.h | 10 ++++++++++
> fs/xfs/xfs_attr_item.c | 31 +++++++------------------------
> 3 files changed, 26 insertions(+), 36 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 7ec3c1e8ea16..e2067aead818 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -499,13 +499,11 @@ int xfs_attr_node_removename_setup(
> ASSERT((*state)->path.blk[(*state)->path.active - 1].magic ==
> XFS_ATTR_LEAF_MAGIC);
>
> - if (args->rmtblkno > 0) {
> - error = xfs_attr_leaf_mark_incomplete(args, *state);
> - if (error)
> - goto out;
> -
> + error = xfs_attr_leaf_mark_incomplete(args, *state);
> + if (error)
> + goto out;
> + if (args->rmtblkno > 0)
> error = xfs_attr_rmtval_invalidate(args);
> - }
> out:
> if (error)
> xfs_da_state_free(*state);
> @@ -778,7 +776,7 @@ xfs_attr_defer_remove(
> if (error)
> return error;
>
> - new->xattri_dela_state = XFS_DAS_UNINIT;
> + new->xattri_dela_state = xfs_attr_init_remove_state(args);
> xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new-
> >xattri_list);
> trace_xfs_attr_defer_remove(new->xattri_dela_state, args->dp);
>
> @@ -1348,16 +1346,15 @@ xfs_attr_node_remove_attr(
> {
> struct xfs_da_args *args = attr->xattri_da_args;
> struct xfs_da_state *state = NULL;
> - struct xfs_mount *mp = args->dp->i_mount;
> int retval = 0;
> int error = 0;
>
> /*
> - * Re-find the "old" attribute entry after any split ops. The
> INCOMPLETE
> - * flag means that we will find the "old" attr, not the "new"
> one.
> + * The attr we are removing has already been marked incomplete,
> so
> + * we need to set the filter appropriately to re-find the "old"
> + * attribute entry after any split ops.
> */
> - if (!xfs_has_larp(mp))
> - args->attr_filter |= XFS_ATTR_INCOMPLETE;
> + args->attr_filter |= XFS_ATTR_INCOMPLETE;
> state = xfs_da_state_alloc(args);
> state->inleaf = 0;
> error = xfs_da3_node_lookup_int(state, &retval);
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 7ea7c7fa31ac..6bef522533a4 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -593,6 +593,16 @@ xfs_attr_init_add_state(struct xfs_da_args
> *args)
> return XFS_DAS_NODE_ADD;
> }
>
> +static inline enum xfs_delattr_state
> +xfs_attr_init_remove_state(struct xfs_da_args *args)
> +{
> + if (xfs_attr_is_shortform(args->dp))
> + return XFS_DAS_SF_REMOVE;
> + if (xfs_attr_is_leaf(args->dp))
> + return XFS_DAS_LEAF_REMOVE;
> + return XFS_DAS_NODE_REMOVE;
> +}
> +
> static inline enum xfs_delattr_state
> xfs_attr_init_replace_state(struct xfs_da_args *args)
> {
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 740a55d07660..fb9549e7ea96 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -296,12 +296,9 @@ xfs_attrd_item_release(
> STATIC int
> xfs_xattri_finish_update(
> struct xfs_attr_item *attr,
> - struct xfs_attrd_log_item *attrdp,
> - uint32_t op_flags)
> + struct xfs_attrd_log_item *attrdp)
> {
> struct xfs_da_args *args = attr->xattri_da_args;
> - unsigned int op = op_flags &
> - XFS_ATTR_OP_FLAGS_TYPE_MAS
> K;
> int error;
>
> if (XFS_TEST_ERROR(false, args->dp->i_mount, XFS_ERRTAG_LARP))
> {
> @@ -309,22 +306,9 @@ xfs_xattri_finish_update(
> goto out;
> }
>
> - switch (op) {
> - case XFS_ATTR_OP_FLAGS_SET:
> - case XFS_ATTR_OP_FLAGS_REPLACE:
> - error = xfs_attr_set_iter(attr);
> - if (!error && attr->xattri_dela_state != XFS_DAS_DONE)
> - error = -EAGAIN;
> - break;
> - case XFS_ATTR_OP_FLAGS_REMOVE:
> - ASSERT(XFS_IFORK_Q(args->dp));
> - error = xfs_attr_remove_iter(attr);
> - break;
> - default:
> - error = -EFSCORRUPTED;
> - break;
> - }
> -
> + error = xfs_attr_set_iter(attr);
> + if (!error && attr->xattri_dela_state != XFS_DAS_DONE)
> + error = -EAGAIN;
> out:
> /*
> * Mark the transaction dirty, even on error. This ensures the
> @@ -432,8 +416,7 @@ xfs_attr_finish_item(
> */
> attr->xattri_da_args->trans = tp;
>
> - error = xfs_xattri_finish_update(attr, done_item,
> - attr->xattri_op_flags);
> + error = xfs_xattri_finish_update(attr, done_item);
> if (error != -EAGAIN)
> kmem_free(attr);
>
> @@ -581,7 +564,7 @@ xfs_attri_item_recover(
> attr->xattri_dela_state =
> xfs_attr_init_add_state(args);
> break;
> case XFS_ATTR_OP_FLAGS_REMOVE:
> - attr->xattri_dela_state = XFS_DAS_UNINIT;
> + attr->xattri_dela_state =
> xfs_attr_init_remove_state(args);
> break;
> default:
> ASSERT(0);
> @@ -600,7 +583,7 @@ xfs_attri_item_recover(
> xfs_ilock(ip, XFS_ILOCK_EXCL);
> xfs_trans_ijoin(tp, ip, 0);
>
> - ret = xfs_xattri_finish_update(attr, done_item, attrp-
> >alfi_op_flags);
> + ret = xfs_xattri_finish_update(attr, done_item);
> if (ret == -EAGAIN) {
> /* There's more work to do, so add it to this
> transaction */
> xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_ATTR, &attr-
> >xattri_list);
