[PATCH RFC 1/1] xfs: refactor xfs_attr_set follow up

[PATCH RFC 1/1] xfs: refactor xfs_attr_set follow up

[Allison Henderson]

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!
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);
+	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;
+	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;
+	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:
+			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);
+}
+
 /*
  * 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

Re: [PATCH RFC 1/1] xfs: refactor xfs_attr_set follow up

[Brian Foster]

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
> 

Re: [PATCH RFC 1/1] xfs: refactor xfs_attr_set follow up

[Allison Henderson]

On 1/21/21 11:47 AM, Brian Foster wrote:
> 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.
Sure, I think doing quick and dirty patches on top moves a little faster 
just because the infastructure is already setup, and I'm not working 
through merge conflicts, plus it cuts out the userspace side and just 
the misc nits that are probably best done after the greater 
architectural descisions are set.  The idea being to just establish an 
end goal of course.  It is tougher to review like this though, maybe we 
can do a few spins of this and I'll try to break it down into smaller 
chunks if that's ok.


> 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. 
Right, but then the caller lacks enough context to decide which state 
that -EAGAIN is meant to go to.  I think I may do a little more digging 
in this area, I think we may have to further break up the helper, but 
should be do-able.

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?
Sure, will do.  The traces are new from the last review, so they will 
likly still churn some reviews.  But they go in after the refactoring, 
so I'm not too worried about them at this level since we're we're mostly 
focused on re-arranging the statemachine.

> 
>> +	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.
Ok, will add some /* fallthough */ commentary here
> 
>> +	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?
I may have missed this collapse here.  I will see if I can consolodate 
those two

> 
>> +	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.
Yeah, I actually didnt know you could have a case in the middle of a 
scope like that, but I guess that's legal.  I could maybe wrap the below 
in a if (attr->xattri_dela_state == XFS_DAS_ALLOC_NODE).  I think that 
would be logically equivelent
> 
>> +			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.
Yes, I'm thinking maybe I could chop this down into a few patches that 
just sort of hoist everything up and then try to collapse down any 
duplicated code in a successive patch.  It'll create bit of a monster 
function at first, but I think it might help us find the an arrangement 
we like.

Thanks for the reviews!  I know its complicated!!
Allison

> 
> 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
>>
> 

Re: [PATCH RFC 1/1] xfs: refactor xfs_attr_set follow up

[Brian Foster]

On Mon, Jan 25, 2021 at 03:22:59PM -0700, Allison Henderson wrote:
> 
> 
> On 1/21/21 11:47 AM, Brian Foster wrote:
> > 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.
> Sure, I think doing quick and dirty patches on top moves a little faster
> just because the infastructure is already setup, and I'm not working through
> merge conflicts, plus it cuts out the userspace side and just the misc nits
> that are probably best done after the greater architectural descisions are
> set.  The idea being to just establish an end goal of course.  It is tougher
> to review like this though, maybe we can do a few spins of this and I'll try
> to break it down into smaller chunks if that's ok.
> 
> 
> > 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
...
> > > @@ -274,108 +300,197 @@ xfs_attr_set_shortform(
...
> > > +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.
> Yes, I'm thinking maybe I could chop this down into a few patches that just
> sort of hoist everything up and then try to collapse down any duplicated
> code in a successive patch.  It'll create bit of a monster function at
> first, but I think it might help us find the an arrangement we like.
> 

FWIW, I've seen enough that the design seems sane and reasonable to me.
It addresses my primary gripe with the current implementation of
spreading the state management code around a bit too much. My previous
comments on this patch were around some of the detailed state breakdown
and management logic, but I don't think that really requires seeing the
whole thing completely done and functional to resolve. In fact, I'd
rather not get too deep into detailed functional and state review with
an RFC patch based on top of the current implementation, as this patch
is, because that will just have to be repeated when broken down into
smaller patches and rebased.

Of course, that is not to say we shouldn't continue down this RFC path
if you have other reasons to do so, aren't sold on the approach, have
design concerns, etc. I'm not totally clear on where we stand on that
tbh. This is just an FYI that if you want my .02, I think the design is
reasonable and I'd probably reserve detailed review for a proper non-rfc
series.

Brian

> Thanks for the reviews!  I know its complicated!!
> Allison
> 
> > 
> > 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
> > > 
> > 
> 

Re: [PATCH RFC 1/1] xfs: refactor xfs_attr_set follow up

[Allison Henderson]

On 1/27/21 10:57 AM, Brian Foster wrote:
> On Mon, Jan 25, 2021 at 03:22:59PM -0700, Allison Henderson wrote:
>>
>>
>> On 1/21/21 11:47 AM, Brian Foster wrote:
>>> 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.
>> Sure, I think doing quick and dirty patches on top moves a little faster
>> just because the infastructure is already setup, and I'm not working through
>> merge conflicts, plus it cuts out the userspace side and just the misc nits
>> that are probably best done after the greater architectural descisions are
>> set.  The idea being to just establish an end goal of course.  It is tougher
>> to review like this though, maybe we can do a few spins of this and I'll try
>> to break it down into smaller chunks if that's ok.
>>
>>
>>> 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
> ...
>>>> @@ -274,108 +300,197 @@ xfs_attr_set_shortform(
> ...
>>>> +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.
>> Yes, I'm thinking maybe I could chop this down into a few patches that just
>> sort of hoist everything up and then try to collapse down any duplicated
>> code in a successive patch.  It'll create bit of a monster function at
>> first, but I think it might help us find the an arrangement we like.
>>
> 
> FWIW, I've seen enough that the design seems sane and reasonable to me.
> It addresses my primary gripe with the current implementation of
> spreading the state management code around a bit too much. My previous
> comments on this patch were around some of the detailed state breakdown
> and management logic, but I don't think that really requires seeing the
> whole thing completely done and functional to resolve. In fact, I'd
> rather not get too deep into detailed functional and state review with
> an RFC patch based on top of the current implementation, as this patch
> is, because that will just have to be repeated when broken down into
> smaller patches and rebased.
Sure, that's fine. It kind of feels like having a mid way review, but I 
understand it's not the easiest way to review things either :-)

> 
> Of course, that is not to say we shouldn't continue down this RFC path
> if you have other reasons to do so, aren't sold on the approach, have
> design concerns, etc. I'm not totally clear on where we stand on that
> tbh. This is just an FYI that if you want my .02, I think the design is
> reasonable and I'd probably reserve detailed review for a proper non-rfc
> series.
Alrighty, I think I get the idea of what we wanna see, so I move forward 
with a v14.  If I run into a snag, we can come back to the RFCs later. 
Thanks for all the help!

Allison

> 
> Brian
> 
>> Thanks for the reviews!  I know its complicated!!
>> Allison
>>
>>>
>>> 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
>>>>
>>>
>>
>