On Thu, 2022-04-14 at 19:44 +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>
> ---
> fs/xfs/libxfs/xfs_attr.c | 26 +++++++++++++++-----------
> fs/xfs/xfs_attr_item.c | 27 ++++++---------------------
> 2 files changed, 21 insertions(+), 32 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 8665b74ddfaf..ccc72c0c3cf5 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -507,13 +507,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);
> @@ -954,6 +952,13 @@ xfs_attr_remove_deferred(
> if (error)
> return error;
>
> + if (xfs_attr_is_shortform(args->dp))
> + new->xattri_dela_state = XFS_DAS_SF_REMOVE;
> + else if (xfs_attr_is_leaf(args->dp))
> + new->xattri_dela_state = XFS_DAS_LEAF_REMOVE;
> + else
> + new->xattri_dela_state = XFS_DAS_NODE_REMOVE;
> +
Mmmm, same issue here as in patch 4, this initial state configs would
get missed during a replay since these routines are only used in the
delayed attr code path, not the replay code path.
> xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new-
> >xattri_list);
>
> return 0;
> @@ -1342,16 +1347,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/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index f2de86756287..39af681897a2 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -298,12 +298,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))
> {
> @@ -311,20 +308,9 @@ xfs_xattri_finish_update(
> goto out;
> }
>
> - switch (op) {
> - case XFS_ATTR_OP_FLAGS_SET:
> - 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;
>
The concern I have here is that op_flags is recorded and recovered from
the log item (see xfs_attri_item_recover). Where as xattri_dela_state
is not. What ever value was set in attr before the shut down would not
be there upon recovery, and with out op_flag we wont know how to
configure it.
I think maybe what you meant to do is log the state? We need to get
into xfs_attri_log_format and turn alfi_op_flags into
a alfi_dela_state. I think that would sew together what you are trying
to do here?
Allison
> out:
> /*
> @@ -435,8 +421,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);
>
> @@ -586,7 +571,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);
