On Thu, 2022-04-14 at 19:44 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Ground work to enable safe recovery of replace operations.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_attr.c | 139 ++++++++++++++++++++++---------------
> --
> fs/xfs/libxfs/xfs_attr.h | 4 ++
> fs/xfs/xfs_trace.h | 3 +
> 3 files changed, 84 insertions(+), 62 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index a509c998e781..8665b74ddfaf 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -459,6 +459,68 @@ xfs_attr_rmtval_alloc(
> return error;
> }
>
> +/*
> + * Mark an attribute entry INCOMPLETE and save pointers to the
> relevant buffers
> + * for later deletion of the entry.
> + */
> +static int
> +xfs_attr_leaf_mark_incomplete(
> + struct xfs_da_args *args,
> + struct xfs_da_state *state)
> +{
> + int error;
> +
> + /*
> + * Fill in disk block numbers in the state structure
> + * so that we can get the buffers back after we commit
> + * several transactions in the following calls.
> + */
> + error = xfs_attr_fillstate(state);
> + if (error)
> + return error;
> +
> + /*
> + * Mark the attribute as INCOMPLETE
> + */
> + return xfs_attr3_leaf_setflag(args);
> +}
> +
> +/*
> + * Initial setup for xfs_attr_node_removename. Make sure the attr
> is there and
> + * the blocks are valid. Attr keys with remote blocks will be
> marked
> + * incomplete.
> + */
> +static
> +int xfs_attr_node_removename_setup(
> + struct xfs_attr_item *attr)
> +{
> + struct xfs_da_args *args = attr->xattri_da_args;
> + struct xfs_da_state **state = &attr-
> >xattri_da_state;
> + int error;
> +
> + error = xfs_attr_node_hasname(args, state);
> + if (error != -EEXIST)
> + goto out;
> + error = 0;
> +
> + ASSERT((*state)->path.blk[(*state)->path.active - 1].bp !=
> NULL);
> + 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_rmtval_invalidate(args);
> + }
> +out:
> + if (error)
> + xfs_da_state_free(*state);
> +
> + return error;
> +}
> +
> /*
> * Remove the original attr we have just replaced. This is dependent
> on the
> * original lookup and insert placing the old attr in args-
> >blkno/args->index
> @@ -517,6 +579,21 @@ xfs_attr_set_iter(
> case XFS_DAS_NODE_ADD:
> return xfs_attr_node_addname(attr);
>
> + case XFS_DAS_SF_REMOVE:
> + attr->xattri_dela_state = XFS_DAS_DONE;
> + return xfs_attr_sf_removename(args);
> + case XFS_DAS_LEAF_REMOVE:
> + attr->xattri_dela_state = XFS_DAS_DONE;
> + return xfs_attr_leaf_removename(args);
> + case XFS_DAS_NODE_REMOVE:
> + error = xfs_attr_node_removename_setup(attr);
> + if (error)
> + return error;
> + attr->xattri_dela_state = XFS_DAS_NODE_REMOVE_RMT;
> + if (args->rmtblkno == 0)
> + attr->xattri_dela_state++;
> + break;
> +
Ok... it took me a little bit to understand why this was here, but it
makes sense after having skipped ahead to the next patch. We're combining the add/remove code paths to better manage rename for journal replays. Probably just add a blurb at the top?
"We need to merge the add and remove code paths to enable safe recovery
of replace operations. Hoist the initial remove states from
xfs_attr_remove_iter into xfs_attr_set_iter. We will make use of them
in the next patches."
I think the rest looks ok though.
Reviewed-by: Allison Henderson<allison.henderson@xxxxxxxxxx>
> case XFS_DAS_LEAF_SET_RMT:
> case XFS_DAS_NODE_SET_RMT:
> error = xfs_attr_rmtval_find_space(attr);
> @@ -1334,68 +1411,6 @@ xfs_attr_node_shrink(
> return error;
> }
>
> -/*
> - * Mark an attribute entry INCOMPLETE and save pointers to the
> relevant buffers
> - * for later deletion of the entry.
> - */
> -STATIC int
> -xfs_attr_leaf_mark_incomplete(
> - struct xfs_da_args *args,
> - struct xfs_da_state *state)
> -{
> - int error;
> -
> - /*
> - * Fill in disk block numbers in the state structure
> - * so that we can get the buffers back after we commit
> - * several transactions in the following calls.
> - */
> - error = xfs_attr_fillstate(state);
> - if (error)
> - return error;
> -
> - /*
> - * Mark the attribute as INCOMPLETE
> - */
> - return xfs_attr3_leaf_setflag(args);
> -}
> -
> -/*
> - * Initial setup for xfs_attr_node_removename. Make sure the attr
> is there and
> - * the blocks are valid. Attr keys with remote blocks will be
> marked
> - * incomplete.
> - */
> -STATIC
> -int xfs_attr_node_removename_setup(
> - struct xfs_attr_item *attr)
> -{
> - struct xfs_da_args *args = attr->xattri_da_args;
> - struct xfs_da_state **state = &attr-
> >xattri_da_state;
> - int error;
> -
> - error = xfs_attr_node_hasname(args, state);
> - if (error != -EEXIST)
> - goto out;
> - error = 0;
> -
> - ASSERT((*state)->path.blk[(*state)->path.active - 1].bp !=
> NULL);
> - 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_rmtval_invalidate(args);
> - }
> -out:
> - if (error)
> - xfs_da_state_free(*state);
> -
> - return error;
> -}
> -
> STATIC int
> xfs_attr_node_removename(
> struct xfs_da_args *args,
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index f4f78d841857..e4b11ac243d7 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -451,6 +451,10 @@ enum xfs_delattr_state {
> XFS_DAS_RM_NAME, /* Remove attr name */
> XFS_DAS_RM_SHRINK, /* We are shrinking the tree
> */
>
> + XFS_DAS_SF_REMOVE, /* Initial shortform set iter
> state */
> + XFS_DAS_LEAF_REMOVE, /* Initial leaf form set iter
> state */
> + XFS_DAS_NODE_REMOVE, /* Initial node form set iter
> state */
> +
> /* Leaf state set/replace sequence */
> XFS_DAS_LEAF_SET_RMT, /* set a remote xattr from a
> leaf */
> XFS_DAS_LEAF_ALLOC_RMT, /* We are allocating remote
> blocks */
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 3a215d298e62..c85bab6215e1 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -4105,6 +4105,9 @@ TRACE_DEFINE_ENUM(XFS_DAS_NODE_ADD);
> TRACE_DEFINE_ENUM(XFS_DAS_RMTBLK);
> TRACE_DEFINE_ENUM(XFS_DAS_RM_NAME);
> TRACE_DEFINE_ENUM(XFS_DAS_RM_SHRINK);
> +TRACE_DEFINE_ENUM(XFS_DAS_SF_REMOVE);
> +TRACE_DEFINE_ENUM(XFS_DAS_LEAF_REMOVE);
> +TRACE_DEFINE_ENUM(XFS_DAS_NODE_REMOVE);
> TRACE_DEFINE_ENUM(XFS_DAS_LEAF_SET_RMT);
> TRACE_DEFINE_ENUM(XFS_DAS_LEAF_ALLOC_RMT);
> TRACE_DEFINE_ENUM(XFS_DAS_LEAF_REPLACE);
