On Sat, Jan 16, 2021 at 01:12:40AM -0700, Allison Henderson wrote:
> Hi all,
>
> This is a follow up to Brians earlier patch
> "[PATCH RFC] xfs: refactor xfs_attr_set() into incremental components"
>
> This patch resembles the earlier patch, but it is seated at the top of
> the parent pointers set rather than the bottom to give a better
> illustraion of what this approach might end up looking like in the
> bigger picture. This patch is both compiled and tested, and is meant to
> be more of an exploratory effort than anything.
>
> Most of the state management is collapsed into the *_iter functions
> similar to Brians patch which collapsed them into the *_args routines.
> Though there are a few states that a still in a few subfunctions.
>
> In anycase, I think it gives decent idea of what the solution might
> look like in practice. Questions, comments and feedback appreciated.
>
Thanks for the patch. By and large, I think the centralized state
management of __xfs_attr_set_iter() is much more clear than the label
management approach of jumping up and down through multiple levels of
helper functions. For the most part, I'm able to walk through the iter
function and follow the sequence of steps involved in the set. I did
have some higher level comments on various parts of the patch,
particularly where we seem to deviate from centralized state management.
Note that if we were to take this approach, a primary goal was to
incrementally port the existing xfs_attr_set_args() implementation into
states. For example, such that we could split the current monstrous
xfs_attr_set() patch into multiple patches that introduce infrastructure
first, and then convert the existing code a state or so at a time. That
eliminates churn from factoring code into one scheme only to immediately
refactor into another. It also facilitates testing because I think the
rework should be able to maintain functionality across each step.
Porting on top of the whole thing certainly helps to get an advanced
look at the final result. However, if we do use this approach and start
getting into the details of individual states and whatnot, I do think it
would be better to start breaking things down into smaller patches that
replace some of the earlier code rather than use it as a baseline.
Further comments inline...
> Thanks!
> Allison
>
> ---
> fs/xfs/libxfs/xfs_attr.c | 596 +++++++++++++++++++----------------------------
> fs/xfs/libxfs/xfs_attr.h | 4 +-
> 2 files changed, 247 insertions(+), 353 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 6ba8f4b..356e35c 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -45,8 +45,8 @@ STATIC int xfs_attr_shortform_addname(xfs_da_args_t *args);
> /*
> * Internal routines when attribute list is one block.
> */
> +STATIC void xfs_attr_restore_rmt_blk(struct xfs_da_args *args);
> STATIC int xfs_attr_leaf_get(xfs_da_args_t *args);
> -STATIC int xfs_attr_leaf_addname(struct xfs_attr_item *attr);
> 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);
>
> @@ -55,6 +55,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
> */
> STATIC int xfs_attr_node_get(xfs_da_args_t *args);
> STATIC int xfs_attr_node_addname(struct xfs_attr_item *attr);
> +STATIC int xfs_attr_node_addname_work(struct xfs_attr_item *attr);
> STATIC int xfs_attr_node_removename_iter(struct xfs_attr_item *attr);
> STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
> struct xfs_da_state **state);
> @@ -219,52 +220,77 @@ xfs_attr_is_shortform(
> ip->i_afp->if_nextents == 0);
> }
>
> -/*
> - * Attempts to set an attr in shortform, or converts short form to leaf form if
> - * there is not enough room. This function is meant to operate as a helper
> - * routine to the delayed attribute functions. It returns -EAGAIN to indicate
> - * that the calling function should roll the transaction, and then proceed to
> - * add the attr in leaf form. This subroutine does not expect to be recalled
> - * again like the other delayed attr routines do.
> - */
> -STATIC int
> -xfs_attr_set_shortform(
> - struct xfs_da_args *args,
> - struct xfs_buf **leaf_bp)
> +int
> +xfs_attr_set_fmt(
> + struct xfs_attr_item *attr,
> + bool *done)
> {
> + struct xfs_da_args *args = attr->xattri_da_args;
> struct xfs_inode *dp = args->dp;
> + struct xfs_buf **leaf_bp = &attr->xattri_leaf_bp;
> int error = 0;
>
> - /*
> - * Try to add the attr to the attribute list in the inode.
> - */
> - error = xfs_attr_try_sf_addname(dp, args);
> + *done = false;
> + if (xfs_attr_is_shortform(dp)) {
>
> - /* Should only be 0, -EEXIST or -ENOSPC */
> - if (error != -ENOSPC) {
> - return error;
> + *done = true;
> + error = xfs_attr_try_sf_addname(dp, args);
> + if (!error)
> + *done = true;
> +
> + if (error != -ENOSPC)
> + return error;
> +
> + error = xfs_attr_shortform_to_leaf(args, leaf_bp);
> + if (error)
> + return error;
> +
> + xfs_trans_bhold(args->trans, *leaf_bp);
> +
> + trace_xfs_das_state_return(XFS_DAS_UNINIT);
> + return -EAGAIN;
> }
> /*
> - * It won't fit in the shortform, transform to a leaf block. GROT:
> - * another possible req'mt for a double-split btree op.
> + * After a shortform to leaf conversion, we need to hold the leaf and
> + * cycle out the transaction. When we get back, we need to release
> + * the leaf to release the hold on the leaf buffer.
> */
> - error = xfs_attr_shortform_to_leaf(args, leaf_bp);
> - if (error)
> - return error;
> + if (*leaf_bp != NULL) {
> + xfs_trans_bhold_release(args->trans, *leaf_bp);
> + *leaf_bp = NULL;
> + }
>
> - /*
> - * Prevent the leaf buffer from being unlocked so that a concurrent AIL
> - * push cannot grab the half-baked leaf buffer and run into problems
> - * with the write verifier.
> - */
> - xfs_trans_bhold(args->trans, *leaf_bp);
> + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> + error = xfs_attr_leaf_try_add(args, *leaf_bp);
> + switch (error) {
> + case -ENOSPC:
> + /*
> + * Promote the attribute list to the Btree format.
> + */
> + error = xfs_attr3_leaf_to_node(args);
> + if (error)
> + return error;
>
> - /*
> - * We're still in XFS_DAS_UNINIT state here. We've converted the attr
> - * fork to leaf format and will restart with the leaf add.
> - */
> - trace_xfs_das_state_return(XFS_DAS_UNINIT);
> - return -EAGAIN;
> + /*
> + * Finish any deferred work items and roll the
> + * transaction once more. The goal here is to call
> + * node_addname with the inode and transaction in the
> + * same state (inode locked and joined, transaction
> + * clean) no matter how we got to this step.
> + *
> + * At this point, we are still in XFS_DAS_UNINIT, but
> + * when we come back, we'll be a node, so we'll fall
> + * down into the node handling code below
> + */
> + trace_xfs_das_state_return(attr->xattri_dela_state);
> + return -EAGAIN;
> + case 0:
> + attr->xattri_dela_state = XFS_DAS_FOUND_LBLK;
> + trace_xfs_das_state_return(attr->xattri_dela_state);
> + return -EAGAIN;
> + }
> + }
> + return error;
> }
>
> /*
> @@ -274,108 +300,197 @@ xfs_attr_set_shortform(
> * to handle this, and recall the function until a successful error code is
> * returned.
> */
> -int
> -xfs_attr_set_iter(
> - struct xfs_attr_item *attr)
> +STATIC int
> +__xfs_attr_set_iter(
> + struct xfs_attr_item *attr,
> + bool *done)
> {
> struct xfs_da_args *args = attr->xattri_da_args;
> - struct xfs_inode *dp = args->dp;
> - struct xfs_buf **leaf_bp = &attr->xattri_leaf_bp;
> - int error = 0;
> int sf_size;
> + struct xfs_buf *bp = NULL;
> + int error, forkoff;
> + struct xfs_inode *dp = args->dp;
> + struct xfs_mount *mp = args->dp->i_mount;
>
> /* State machine switch */
> switch (attr->xattri_dela_state) {
> - case XFS_DAS_FLIP_LFLAG:
> - case XFS_DAS_FOUND_LBLK:
> - case XFS_DAS_RM_LBLK:
> - return xfs_attr_leaf_addname(attr);
> - case XFS_DAS_FOUND_NBLK:
> - case XFS_DAS_FLIP_NFLAG:
> - case XFS_DAS_ALLOC_NODE:
> - return xfs_attr_node_addname(attr);
> case XFS_DAS_UNINIT:
> - break;
> - default:
> - ASSERT(attr->xattri_dela_state != XFS_DAS_RM_SHRINK);
> - break;
> - }
> + if (XFS_IFORK_Q((args->dp)) == 0) {
> + sf_size = sizeof(struct xfs_attr_sf_hdr) +
> + xfs_attr_sf_entsize_byname(args->namelen,
> + args->valuelen);
> + xfs_bmap_set_attrforkoff(args->dp, sf_size, NULL);
> + args->dp->i_afp = kmem_cache_zalloc(xfs_ifork_zone, 0);
> + args->dp->i_afp->if_flags = XFS_IFEXTENTS;
> + args->dp->i_afp->if_format = XFS_DINODE_FMT_EXTENTS;
> + }
>
> - /*
> - * New inodes may not have an attribute fork yet. So set the attribute
> - * fork appropriately
> - */
> - if (XFS_IFORK_Q((args->dp)) == 0) {
> - sf_size = sizeof(struct xfs_attr_sf_hdr) +
> - xfs_attr_sf_entsize_byname(args->namelen,
> - args->valuelen);
> - xfs_bmap_set_attrforkoff(args->dp, sf_size, NULL);
> - args->dp->i_afp = kmem_cache_zalloc(xfs_ifork_zone, 0);
> - args->dp->i_afp->if_flags = XFS_IFEXTENTS;
> - args->dp->i_afp->if_format = XFS_DINODE_FMT_EXTENTS;
> - }
> + return xfs_attr_set_fmt(attr, done);
One thing the original patch tried to accomplish was to draw a line
between the state management and underlying helpers in this iter()
function. It looks like you've done that in other places, but here some
of that state code is buried down in _set_fmt(). Instead, I think the
helper should return -EAGAIN only if it should be called again and
otherwise return 0. The caller then determines if/how/when to move to
the next state. The purpose of the done flag in this case was to let the
caller know whether the operation had completed or to move on to the
next state.
Also, I notice a lot of
trace_xfs_das_state_return(attr->xattri_dela_state) calls scattered
about. Could we turn that into a common exit path out of this function
and let the users fall into it appropriately with a label?
> + case XFS_DAS_FOUND_LBLK:
> + if (args->rmtblkno > 0) {
> + error = xfs_attr_rmtval_find_space(attr);
> + if (error)
> + return error;
> + }
> + attr->xattri_dela_state = XFS_DAS_ALLOC_LBLK;
Just FWIW, I get a bunch of warnings related to these fallthrus with gcc
10.2.1.
> + case XFS_DAS_ALLOC_LBLK:
> + if (attr->xattri_blkcnt > 0) {
> + error = xfs_attr_rmtval_set_blk(attr);
> + if (error)
> + return error;
>
> - /*
> - * If the attribute list is already in leaf format, jump straight to
> - * leaf handling. Otherwise, try to add the attribute to the shortform
> - * list; if there's no room then convert the list to leaf format and try
> - * again. No need to set state as we will be in leaf form when we come
> - * back
> - */
> - if (xfs_attr_is_shortform(dp)) {
> + trace_xfs_das_state_return(attr->xattri_dela_state);
> + return -EAGAIN;
> + }
> + attr->xattri_dela_state = XFS_DAS_SET_LBLK;
> + case XFS_DAS_SET_LBLK:
> + error = xfs_attr_rmtval_set_value(args);
> + if (error)
> + return error;
>
> - /*
> - * If the attr was successfully set in shortform, no need to
> - * continue. Otherwise, is it converted from shortform to leaf
> - * and -EAGAIN is returned.
> - */
> - return xfs_attr_set_shortform(args, leaf_bp);
> - }
> + if (!(args->op_flags & XFS_DA_OP_RENAME)) {
> + /*
> + * Added a "remote" value, just clear the incomplete flag.
> + */
> + if (args->rmtblkno > 0)
> + error = xfs_attr3_leaf_clearflag(args);
>
> - /*
> - * After a shortform to leaf conversion, we need to hold the leaf and
> - * cycle out the transaction. When we get back, we need to release
> - * the leaf to release the hold on the leaf buffer.
> - */
> - if (*leaf_bp != NULL) {
> - xfs_trans_bhold_release(args->trans, *leaf_bp);
> - *leaf_bp = NULL;
> - }
> + return error;
> + }
>
> - if (!xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> - return xfs_attr_node_addname(attr);
> + if (xfs_hasdelattr(mp))
> + return error;
>
> - error = xfs_attr_leaf_try_add(args, *leaf_bp);
> - switch (error) {
> - case -ENOSPC:
> + error = xfs_attr3_leaf_flipflags(args);
> + if (error)
> + return error;
> /*
> - * Promote the attribute list to the Btree format.
> + * Commit the flag value change and start the next trans in series.
> */
> - error = xfs_attr3_leaf_to_node(args);
> + attr->xattri_dela_state = XFS_DAS_INVAL_LBLK;
> + trace_xfs_das_state_return(attr->xattri_dela_state);
> + return -EAGAIN;
> +
> + case XFS_DAS_INVAL_LBLK:
> + xfs_attr_restore_rmt_blk(args);
> +
> + error = xfs_attr_rmtval_invalidate(args);
> if (error)
> return error;
>
> + attr->xattri_dela_state = XFS_DAS_RM_LBLK;
> + case XFS_DAS_RM_LBLK:
> + if (args->rmtblkno) {
> + error = xfs_attr_rmtval_remove(attr);
> + if (error == -EAGAIN)
> + trace_xfs_das_state_return(attr->xattri_dela_state);
> + if (error)
> + return error;
> + }
> +
> + error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno,
> + &bp);
> + if (error)
> + return error;
> +
> + xfs_attr3_leaf_remove(bp, args);
> +
> + forkoff = xfs_attr_shortform_allfit(bp, dp);
> + if (forkoff)
> + error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
> + /* bp is gone due to xfs_da_shrink_inode */
> +
> + return error;
Is this a completion state? How do end up in the state below?
Hmm.. that comes from xfs_attr_node_addname(), so it looks like this was
split into semi-duplicate states between leaf/node formats. Was there a
reason this was split up instead of folded together as attempted in the
original patch?
> + case XFS_DAS_FOUND_NBLK:
> + if (args->rmtblkno > 0) {
> + error = xfs_attr_rmtval_find_space(attr);
> + if (error)
> + return error;
> +
> + attr->xattri_dela_state = XFS_DAS_ALLOC_NODE;
> + case XFS_DAS_ALLOC_NODE:
Oof, the case statement inside the if branch is rather odd here.
> + if (attr->xattri_blkcnt > 0) {
> + error = xfs_attr_rmtval_set_blk(attr);
> + if (error)
> + return error;
> +
> + trace_xfs_das_state_return(attr->xattri_dela_state);
> + return -EAGAIN;
> + }
> +
> + error = xfs_attr_rmtval_set_value(args);
> + if (error)
> + return error;
> + }
> +
> + if (!(args->op_flags & XFS_DA_OP_RENAME)) {
> + /*
> + * Added a rmt value, just clear the incomplete flag.
> + */
> + if (args->rmtblkno > 0)
> + error = xfs_attr3_leaf_clearflag(args);
> + return error;
> + }
> +
> + if (!xfs_hasdelattr(mp)) {
> + error = xfs_attr3_leaf_flipflags(args);
> + if (error)
> + return error;
> + /*
> + * Commit the flag value change and start the next trans
> + * in series
> + */
> + attr->xattri_dela_state = XFS_DAS_FLIP_NFLAG;
> + trace_xfs_das_state_return(attr->xattri_dela_state);
> + return -EAGAIN;
> + }
> + case XFS_DAS_FLIP_NFLAG:
> /*
> - * Finish any deferred work items and roll the
> - * transaction once more. The goal here is to call
> - * node_addname with the inode and transaction in the
> - * same state (inode locked and joined, transaction
> - * clean) no matter how we got to this step.
> - *
> - * At this point, we are still in XFS_DAS_UNINIT, but
> - * when we come back, we'll be a node, so we'll fall
> - * down into the node handling code below
> + * Dismantle the "old" attribute/value pair by removing a
> + * "remote" value (if it exists).
> */
> - trace_xfs_das_state_return(attr->xattri_dela_state);
> - return -EAGAIN;
> - case 0:
> - attr->xattri_dela_state = XFS_DAS_FOUND_LBLK;
> - trace_xfs_das_state_return(attr->xattri_dela_state);
> - return -EAGAIN;
> + xfs_attr_restore_rmt_blk(args);
> +
> + error = xfs_attr_rmtval_invalidate(args);
> + if (error)
> + return error;
> +
> + attr->xattri_dela_state = XFS_DAS_RM_NBLK;
> + case XFS_DAS_RM_NBLK:
> + if (args->rmtblkno) {
> + error = xfs_attr_rmtval_remove(attr);
> +
> + if (error == -EAGAIN) {
> + attr->xattri_dela_state = XFS_DAS_RM_NBLK;
> + trace_xfs_das_state_return(attr->xattri_dela_state);
> + }
> +
> + if (error)
> + return error;
> + }
> +
> + return xfs_attr_node_addname_work(attr);
> + default:
> + ASSERT(attr->xattri_dela_state != XFS_DAS_RM_SHRINK);
> + break;
> }
> +
> return error;
> }
>
> +int xfs_attr_set_iter(
> + struct xfs_attr_item *attr)
> +{
> + bool done = true;
> + int error;
> +
> + error = __xfs_attr_set_iter(attr, &done);
> + if (error || done)
> + return error;
> +
> + return xfs_attr_node_addname(attr);
Note that this was also intended to go away and get folded into the
state code in __xfs_attr_set_iter(), I just left off here because it
looked like there might have been opportunity to fall into the remove
path, and that was getting a bit more involved than I wanted to. This
variant looks a little different in that we can presumably fall into
this function and then back into the state machine. Even if we didn't
immediately reuse the remove path, I suspect we should probably continue
chunking off the remainder of the operation into proper states.
Brian
> +}
> +
> /*
> * Return EEXIST if attr is found, or ENOATTR if not
> */
> @@ -773,145 +888,6 @@ xfs_attr_leaf_try_add(
>
>
> /*
> - * Add a name to the leaf attribute list structure
> - *
> - * This leaf block cannot have a "remote" value, we only call this routine
> - * if bmap_one_block() says there is only one block (ie: no remote blks).
> - *
> - * This routine is meant to function as a delayed operation, and may return
> - * -EAGAIN when the transaction needs to be rolled. Calling functions will need
> - * to handle this, and recall the function until a successful error code is
> - * returned.
> - */
> -STATIC int
> -xfs_attr_leaf_addname(
> - struct xfs_attr_item *attr)
> -{
> - struct xfs_da_args *args = attr->xattri_da_args;
> - struct xfs_buf *bp = NULL;
> - int error, forkoff;
> - struct xfs_inode *dp = args->dp;
> - struct xfs_mount *mp = args->dp->i_mount;
> -
> - /* State machine switch */
> - switch (attr->xattri_dela_state) {
> - case XFS_DAS_FLIP_LFLAG:
> - goto das_flip_flag;
> - case XFS_DAS_RM_LBLK:
> - goto das_rm_lblk;
> - default:
> - break;
> - }
> -
> - /*
> - * If there was an out-of-line value, allocate the blocks we
> - * identified for its storage and copy the value. This is done
> - * after we create the attribute so that we don't overflow the
> - * maximum size of a transaction and/or hit a deadlock.
> - */
> -
> - /* Open coded xfs_attr_rmtval_set without trans handling */
> - if ((attr->xattri_flags & XFS_DAC_LEAF_ADDNAME_INIT) == 0) {
> - attr->xattri_flags |= XFS_DAC_LEAF_ADDNAME_INIT;
> - if (args->rmtblkno > 0) {
> - error = xfs_attr_rmtval_find_space(attr);
> - if (error)
> - return error;
> - }
> - }
> -
> - /*
> - * Roll through the "value", allocating blocks on disk as
> - * required.
> - */
> - if (attr->xattri_blkcnt > 0) {
> - error = xfs_attr_rmtval_set_blk(attr);
> - if (error)
> - return error;
> -
> - trace_xfs_das_state_return(attr->xattri_dela_state);
> - return -EAGAIN;
> - }
> -
> - error = xfs_attr_rmtval_set_value(args);
> - if (error)
> - return error;
> -
> - if (!(args->op_flags & XFS_DA_OP_RENAME)) {
> - /*
> - * Added a "remote" value, just clear the incomplete flag.
> - */
> - if (args->rmtblkno > 0)
> - error = xfs_attr3_leaf_clearflag(args);
> -
> - return error;
> - }
> -
> - /*
> - * If this is an atomic rename operation, we must "flip" the incomplete
> - * flags on the "new" and "old" attribute/value pairs so that one
> - * disappears and one appears atomically. Then we must remove the "old"
> - * attribute/value pair.
> - *
> - * In a separate transaction, set the incomplete flag on the "old" attr
> - * and clear the incomplete flag on the "new" attr.
> - */
> - if (!xfs_hasdelattr(mp)) {
> - error = xfs_attr3_leaf_flipflags(args);
> - if (error)
> - return error;
> - /*
> - * Commit the flag value change and start the next trans in series.
> - */
> - attr->xattri_dela_state = XFS_DAS_FLIP_LFLAG;
> - trace_xfs_das_state_return(attr->xattri_dela_state);
> - return -EAGAIN;
> - }
> -das_flip_flag:
> - /*
> - * Dismantle the "old" attribute/value pair by removing a "remote" value
> - * (if it exists).
> - */
> - xfs_attr_restore_rmt_blk(args);
> -
> - error = xfs_attr_rmtval_invalidate(args);
> - if (error)
> - return error;
> -
> - /* Set state in case xfs_attr_rmtval_remove returns -EAGAIN */
> - attr->xattri_dela_state = XFS_DAS_RM_LBLK;
> -das_rm_lblk:
> - if (args->rmtblkno) {
> - error = xfs_attr_rmtval_remove(attr);
> - if (error == -EAGAIN)
> - trace_xfs_das_state_return(attr->xattri_dela_state);
> - if (error)
> - return error;
> - }
> -
> - /*
> - * Read in the block containing the "old" attr, then remove the "old"
> - * attr from that block (neat, huh!)
> - */
> - error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno,
> - &bp);
> - if (error)
> - return error;
> -
> - xfs_attr3_leaf_remove(bp, args);
> -
> - /*
> - * If the result is small enough, shrink it all into the inode.
> - */
> - forkoff = xfs_attr_shortform_allfit(bp, dp);
> - if (forkoff)
> - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
> - /* bp is gone due to xfs_da_shrink_inode */
> -
> - return error;
> -}
> -
> -/*
> * Return EEXIST if attr is found, or ENOATTR if not
> */
> STATIC int
> @@ -1065,24 +1041,9 @@ xfs_attr_node_addname(
> struct xfs_da_state_blk *blk;
> int retval = 0;
> int error = 0;
> - struct xfs_mount *mp = args->dp->i_mount;
>
> trace_xfs_attr_node_addname(args);
>
> - /* State machine switch */
> - switch (attr->xattri_dela_state) {
> - case XFS_DAS_FLIP_NFLAG:
> - goto das_flip_flag;
> - case XFS_DAS_FOUND_NBLK:
> - goto das_found_nblk;
> - case XFS_DAS_ALLOC_NODE:
> - goto das_alloc_node;
> - case XFS_DAS_RM_NBLK:
> - goto das_rm_nblk;
> - default:
> - break;
> - }
> -
> /*
> * Search to see if name already exists, and get back a pointer
> * to where it should go.
> @@ -1171,93 +1132,24 @@ xfs_attr_node_addname(
> attr->xattri_dela_state = XFS_DAS_FOUND_NBLK;
> trace_xfs_das_state_return(attr->xattri_dela_state);
> return -EAGAIN;
> -das_found_nblk:
> -
> - /*
> - * If there was an out-of-line value, allocate the blocks we
> - * identified for its storage and copy the value. This is done
> - * after we create the attribute so that we don't overflow the
> - * maximum size of a transaction and/or hit a deadlock.
> - */
> - if (args->rmtblkno > 0) {
> - /* Open coded xfs_attr_rmtval_set without trans handling */
> - error = xfs_attr_rmtval_find_space(attr);
> - if (error)
> - return error;
> -
> - /*
> - * Roll through the "value", allocating blocks on disk as
> - * required. Set the state in case of -EAGAIN return code
> - */
> - attr->xattri_dela_state = XFS_DAS_ALLOC_NODE;
> -das_alloc_node:
> - if (attr->xattri_blkcnt > 0) {
> - error = xfs_attr_rmtval_set_blk(attr);
> - if (error)
> - return error;
> -
> - trace_xfs_das_state_return(attr->xattri_dela_state);
> - return -EAGAIN;
> - }
> -
> - error = xfs_attr_rmtval_set_value(args);
> - if (error)
> - return error;
> - }
> -
> - if (!(args->op_flags & XFS_DA_OP_RENAME)) {
> - /*
> - * Added a "remote" value, just clear the incomplete flag.
> - */
> - if (args->rmtblkno > 0)
> - error = xfs_attr3_leaf_clearflag(args);
> - retval = error;
> - goto out;
> - }
> -
> - /*
> - * If this is an atomic rename operation, we must "flip" the incomplete
> - * flags on the "new" and "old" attribute/value pairs so that one
> - * disappears and one appears atomically. Then we must remove the "old"
> - * attribute/value pair.
> - *
> - * In a separate transaction, set the incomplete flag on the "old" attr
> - * and clear the incomplete flag on the "new" attr.
> - */
> - if (!xfs_hasdelattr(mp)) {
> - error = xfs_attr3_leaf_flipflags(args);
> - if (error)
> - goto out;
> - /*
> - * Commit the flag value change and start the next trans in series
> - */
> - attr->xattri_dela_state = XFS_DAS_FLIP_NFLAG;
> - trace_xfs_das_state_return(attr->xattri_dela_state);
> - return -EAGAIN;
> - }
> -das_flip_flag:
> - /*
> - * Dismantle the "old" attribute/value pair by removing a "remote" value
> - * (if it exists).
> - */
> - xfs_attr_restore_rmt_blk(args);
> +out:
> + if (state)
> + xfs_da_state_free(state);
>
> - error = xfs_attr_rmtval_invalidate(args);
> if (error)
> return error;
> + return retval;
> +}
>
> - /* Set state in case xfs_attr_rmtval_remove returns -EAGAIN */
> - attr->xattri_dela_state = XFS_DAS_RM_NBLK;
> -das_rm_nblk:
> - if (args->rmtblkno) {
> - error = xfs_attr_rmtval_remove(attr);
> -
> - if (error == -EAGAIN)
> - trace_xfs_das_state_return(attr->xattri_dela_state);
> -
> - if (error)
> - return error;
> - }
> +STATIC
> +int xfs_attr_node_addname_work(
> + struct xfs_attr_item *attr)
> +{
> + struct xfs_da_args *args = attr->xattri_da_args;
> + struct xfs_da_state *state = NULL;
> + struct xfs_da_state_blk *blk;
> + int retval = 0;
> + int error = 0;
>
> /*
> * Re-find the "old" attribute entry after any split ops. The INCOMPLETE
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index c80575a..050e5be 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -376,8 +376,10 @@ enum xfs_delattr_state {
> XFS_DAS_UNINIT = 0, /* No state has been set yet */
> XFS_DAS_RM_SHRINK, /* We are shrinking the tree */
> XFS_DAS_FOUND_LBLK, /* We found leaf blk for attr */
> + XFS_DAS_ALLOC_LBLK,
> + XFS_DAS_SET_LBLK,
> XFS_DAS_FOUND_NBLK, /* We found node blk for attr */
> - XFS_DAS_FLIP_LFLAG, /* Flipped leaf INCOMPLETE attr flag */
> + XFS_DAS_INVAL_LBLK, /* Invalidate leaf blks */
> XFS_DAS_RM_LBLK, /* A rename is removing leaf blocks */
> XFS_DAS_ALLOC_NODE, /* We are allocating node blocks */
> XFS_DAS_FLIP_NFLAG, /* Flipped node INCOMPLETE attr flag */
> --
> 2.7.4
>
