Re: [PATCH v9 24/43] xfsprogs: Split apart xfs_attr_leaf_addname

Allison Collins <allison.henderson@xxxxxxxxxx> · Mon, 4 May 2020 15:33:08 -0700

On 5/4/20 10:32 AM, Darrick J. Wong wrote:
On Thu, Apr 30, 2020 at 03:46:41PM -0700, Allison Collins wrote:
Split out new helper function xfs_attr_leaf_try_add from
xfs_attr_leaf_addname. Because new delayed attribute routines cannot
roll transactions, we split off the parts of xfs_attr_leaf_addname that
we can use, and move the commit into the calling function.

Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx>
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
Reviewed-by: Chandan Rajendra <chandanrlinux@xxxxxxxxx>
---
  libxfs/xfs_attr.c | 94 +++++++++++++++++++++++++++++++++++--------------------
  1 file changed, 60 insertions(+), 34 deletions(-)

diff --git a/libxfs/xfs_attr.c b/libxfs/xfs_attr.c
index 1cdebec..5f622c8 100644
--- a/libxfs/xfs_attr.c
+++ b/libxfs/xfs_attr.c
@@ -257,10 +257,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;

I was just about to debate myself (again) on why it's still necessary to
call _defer_finish and then _trans_roll_inode, but then I remembered the
actual reason I unearthed for that, like 5 revisions ago.

xfs_defer_trans_roll looks for all buffer and inode items attached to a
transaction.  If it finds bhold'ed buffers or inodes that were ijoined
with lock_flags==0, it will log and rejoin those items to the new
transaction after rolling.  Therefore, the xfs_trans_roll_inode here
makes sure that any buffers that might have been bhold'ed have been
released.  The goal here is to start each step of an xattr operation
with the transaction in the same state (clean transaction, inode locked
and joined) no matter how we got to that step.  That's what "manage its
own transactions" means, though that's apparently too vague for me to
pick up on.

With that in mind, could you change the comments for both to state why
we're doing this more explicitly? e.g.

	/*
	 * 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.
	 */
	error = xfs_defer_finish(&args->trans);
	if (error)
		return error;
	error = xfs_trans_roll_inode(&args->trans, dp);
	if (error)
		return error;

+

Unnecessary blank line here.

Sure, will fix these.  Thanks for the reviews!

Allison

--D

+	}
+
+	error = xfs_attr_node_addname(args);
  	return error;
  }
  
@@ -508,20 +528,21 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
   *========================================================================*/
  
  /*
- * Add a name to the leaf attribute list structure
+ * Tries to add an attribute to an inode in leaf form
   *
- * 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 function is meant to execute as part of a delayed operation and leaves
+ * the transaction handling to the caller.  On success the attribute is added
+ * and the inode and transaction are left dirty.  If there is not enough space,
+ * the attr data is converted to node format and -ENOSPC is returned. Caller is
+ * responsible for handling the dirty inode and transaction or adding the attr
+ * in node format.
   */
  STATIC int
-xfs_attr_leaf_addname(
-	struct xfs_da_args	*args)
+xfs_attr_leaf_try_add(
+	struct xfs_da_args	*args,
+	struct xfs_buf		*bp)
  {
-	struct xfs_buf		*bp;
-	int			retval, error, forkoff;
-	struct xfs_inode	*dp = args->dp;
-
-	trace_xfs_attr_leaf_addname(args);
+	int			retval, error;
  
  	/*
  	 * Look up the given attribute in the leaf block.  Figure out if
@@ -563,31 +584,39 @@ 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
  		 */
  		error = xfs_attr3_leaf_to_node(args);
  		if (error)
  			return error;
-		error = xfs_defer_finish(&args->trans);
-		if (error)
-			return error;
+	}
+	return retval;
+out_brelse:
+	xfs_trans_brelse(args->trans, bp);
+	return retval;
+}
  
-		/*
-		 * Commit the current trans (including the inode) and start
-		 * a new one.
-		 */
-		error = xfs_trans_roll_inode(&args->trans, dp);
-		if (error)
-			return error;
  
-		/*
-		 * Fob the whole rest of the problem off on the Btree code.
-		 */
-		error = xfs_attr_node_addname(args);
+/*
+ * 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).
+ */
+STATIC int
+xfs_attr_leaf_addname(
+	struct xfs_da_args	*args)
+{
+	int			error, forkoff;
+	struct xfs_buf		*bp = NULL;
+	struct xfs_inode	*dp = args->dp;
+
+	trace_xfs_attr_leaf_addname(args);
+
+	error = xfs_attr_leaf_try_add(args, bp);
+	if (error)
  		return error;
-	}
  
  	/*
  	 * Commit the transaction that added the attr name so that
@@ -682,9 +711,6 @@ xfs_attr_leaf_addname(
  		error = xfs_attr3_leaf_clearflag(args);
  	}
  	return error;
-out_brelse:
-	xfs_trans_brelse(args->trans, bp);
-	return retval;
  }
  
  /*
--
2.7.4







