On 2/24/20 11:42 PM, Dave Chinner wrote:

On Sat, Feb 22, 2020 at 07:05:59PM -0700, Allison Collins wrote:

quoted

Factor out new helper function xfs_attr_leaf_try_add. Because new delayed attribute
routines cannot roll transactions, we carve off the parts of xfs_attr_leaf_addname
that we can use, and move the commit into the calling function.

68-72 columns :P

Yes, I had adjusted the configs on the editor and thought I had it 
fixed.  Will fix

quoted

Signed-off-by: Allison Collins <redacted>
Reviewed-by: Brian Foster <redacted>
---
  fs/xfs/libxfs/xfs_attr.c | 88 +++++++++++++++++++++++++++++++-----------------
  1 file changed, 57 insertions(+), 31 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index cf0cba7..b2f0780 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c

@@ -305,10 +305,30 @@ xfs_attr_set_args(
  		}
  	}
  
-	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
+	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
  		error = xfs_attr_leaf_addname(args);
-	else
-		error = xfs_attr_node_addname(args);
+		if (error != -ENOSPC)
+			return error;
+
+		/*
+		 * Commit that transaction so that the node_addname()
+		 * call can manage its own transactions.
+		 */
+		error = xfs_defer_finish(&args->trans);
+		if (error)
+			return error;
+
+		/*
+		 * Commit the current trans (including the inode) and
+		 * start a new one.
+		 */
+		error = xfs_trans_roll_inode(&args->trans, dp);
+		if (error)
+			return error;
+
+	}
+
+	error = xfs_attr_node_addname(args);
  	return error;

	return xfs_attr_node_addname(args);

better yet:

	if (!xfs_bmap_one_block(dp, XFS_ATTR_FORK))
		return xfs_attr_node_addname(args);

	error = xfs_attr_leaf_addname(args);
	if (error != -ENOSPC)
		return error;
	.....

Sure, let me dig around first though, because that's going to move 
around where the states appear in patch 14.  The set is admittedly a bit 
of a balancing act :-)

BTW, I think I see the pattern now - isolate all the metadata
changes from the mechanism of rolling the transactions, which means
it can be converted to a set of operations connected by a generic
transaction rolling mechanism. It's all starting to make more sense
now :P

Alrighty, I'm glad it's coming together. :-)

quoted

@@ -679,31 +700,36 @@ xfs_attr_leaf_addname(
  	retval = xfs_attr3_leaf_add(bp, args);
  	if (retval == -ENOSPC) {
  		/*
-		 * Promote the attribute list to the Btree format, then
-		 * Commit that transaction so that the node_addname() call
-		 * can manage its own transactions.
+		 * Promote the attribute list to the Btree format.
+		 * Unless an error occurs, retain the -ENOSPC retval
  		 */

Comments should use all 80 columns...

Will fix

Thanks for the reviews!
Allison

Cheers,

Dave.