Re: [PATCH v13 03/10] xfs: Add delay ready attr set routines

Allison Henderson <allison.henderson@xxxxxxxxxx> · Fri, 13 Nov 2020 10:12:09 -0700

On 11/13/20 2:16 AM, Chandan Babu R wrote:
On Friday 13 November 2020 7:03:13 AM IST Allison Henderson wrote:

On 11/10/20 2:57 PM, Darrick J. Wong wrote:
On Tue, Oct 27, 2020 at 07:02:55PM +0530, Chandan Babu R wrote:
On Friday 23 October 2020 12:04:28 PM IST Allison Henderson wrote:
This patch modifies the attr set routines to be delay ready. This means
they no longer roll or commit transactions, but instead return -EAGAIN
to have the calling routine roll and refresh the transaction.  In this
series, xfs_attr_set_args has become xfs_attr_set_iter, which uses a
state machine like switch to keep track of where it was when EAGAIN was
returned. See xfs_attr.h for a more detailed diagram of the states.

Two new helper functions have been added: xfs_attr_rmtval_set_init and
xfs_attr_rmtval_set_blk.  They provide a subset of logic similar to
xfs_attr_rmtval_set, but they store the current block in the delay attr
context to allow the caller to roll the transaction between allocations.
This helps to simplify and consolidate code used by
xfs_attr_leaf_addname and xfs_attr_node_addname. xfs_attr_set_args has
now become a simple loop to refresh the transaction until the operation
is completed.  Lastly, xfs_attr_rmtval_remove is no longer used, and is
removed.

One nit. xfs_attr_rmtval_remove()'s prototype declaration needs to be removed
from xfs_attr_remote.h.
Alrighty, will pull out



Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
---
   fs/xfs/libxfs/xfs_attr.c        | 370 ++++++++++++++++++++++++++--------------
   fs/xfs/libxfs/xfs_attr.h        | 126 +++++++++++++-
   fs/xfs/libxfs/xfs_attr_remote.c |  99 +++++++----
   fs/xfs/libxfs/xfs_attr_remote.h |   4 +
   fs/xfs/xfs_trace.h              |   1 -
   5 files changed, 439 insertions(+), 161 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 6ca94cb..95c98d7 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -44,7 +44,7 @@ STATIC int xfs_attr_shortform_addname(xfs_da_args_t *args);
    * Internal routines when attribute list is one block.
    */
   STATIC int xfs_attr_leaf_get(xfs_da_args_t *args);
-STATIC int xfs_attr_leaf_addname(xfs_da_args_t *args);
+STATIC int xfs_attr_leaf_addname(struct xfs_delattr_context *dac);
   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);
   
@@ -52,12 +52,15 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
    * Internal routines when attribute list is more than one block.
    */
   STATIC int xfs_attr_node_get(xfs_da_args_t *args);
-STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
+STATIC int xfs_attr_node_addname(struct xfs_delattr_context *dac);
   STATIC int xfs_attr_node_removename_iter(struct xfs_delattr_context *dac);
   STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
   				 struct xfs_da_state **state);
   STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
   STATIC int xfs_attr_refillstate(xfs_da_state_t *state);
+STATIC int xfs_attr_leaf_try_add(struct xfs_da_args *args, struct xfs_buf *bp);
+STATIC int xfs_attr_set_iter(struct xfs_delattr_context *dac,
+			     struct xfs_buf **leaf_bp);
   
   int
   xfs_inode_hasattr(
@@ -218,8 +221,11 @@ xfs_attr_is_shortform(
   
   /*
    * Attempts to set an attr in shortform, or converts short form to leaf form if
- * there is not enough room.  If the attr is set, the transaction is committed
- * and set to NULL.
+ * 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(
@@ -227,16 +233,16 @@ xfs_attr_set_shortform(
   	struct xfs_buf		**leaf_bp)
   {
   	struct xfs_inode	*dp = args->dp;
-	int			error, error2 = 0;
+	int			error = 0;
   
   	/*
   	 * Try to add the attr to the attribute list in the inode.
   	 */
   	error = xfs_attr_try_sf_addname(dp, args);
+
+	/* Should only be 0, -EEXIST or ENOSPC */
   	if (error != -ENOSPC) {
-		error2 = xfs_trans_commit(args->trans);
-		args->trans = NULL;
-		return error ? error : error2;
+		return error;
   	}
   	/*
   	 * It won't fit in the shortform, transform to a leaf block.  GROT:
@@ -249,18 +255,10 @@ xfs_attr_set_shortform(
   	/*
   	 * 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. Once we're done rolling the transaction we
-	 * can release the hold and add the attr to the leaf.
+	 * with the write verifier.
   	 */
   	xfs_trans_bhold(args->trans, *leaf_bp);
-	error = xfs_defer_finish(&args->trans);
-	xfs_trans_bhold_release(args->trans, *leaf_bp);
-	if (error) {
-		xfs_trans_brelse(args->trans, *leaf_bp);
-		return error;
-	}
-
-	return 0;
+	return -EAGAIN;
   }
   
   /*
@@ -268,7 +266,7 @@ xfs_attr_set_shortform(
    * also checks for a defer finish.  Transaction is finished and rolled as
    * needed, and returns true of false if the delayed operation should continue.
    */
-int
+STATIC int
   xfs_attr_trans_roll(
   	struct xfs_delattr_context	*dac)
   {
@@ -297,61 +295,130 @@ int
   xfs_attr_set_args(
   	struct xfs_da_args	*args)
   {
-	struct xfs_inode	*dp = args->dp;
-	struct xfs_buf          *leaf_bp = NULL;
-	int			error = 0;
+	struct xfs_buf			*leaf_bp = NULL;
+	int				error = 0;
+	struct xfs_delattr_context	dac = {
+		.da_args	= args,
+	};
+
+	do {
+		error = xfs_attr_set_iter(&dac, &leaf_bp);
+		if (error != -EAGAIN)
+			break;
+
+		error = xfs_attr_trans_roll(&dac);
+		if (error)
+			return error;
+
+		if (leaf_bp) {
+			xfs_trans_bjoin(args->trans, leaf_bp);
+			xfs_trans_bhold(args->trans, leaf_bp);
+		}

When xfs_attr_set_iter() causes a "short form" attribute list to be converted
to "leaf form", leaf_bp would point to an xfs_buf which has been added to the
transaction and also XFS_BLI_HOLD flag is set on the buffer (last statement in
xfs_attr_set_shortform()). XFS_BLI_HOLD flag makes sure that the new
transaction allocated by xfs_attr_trans_roll() would continue to have leaf_bp
in the transaction's item list. Hence I think the above calls to
xfs_trans_bjoin() and xfs_trans_bhold() are not required.
Sorry, I just noticed Chandans commentary for this patch.  Apologies. I
think we can get away with out this now, but yes this routine disappears
at the end of the set now.  Will clean out anyway for bisecting reasons
though. :-)

No problem. As an aside, I stopped reviewing the patchset after I noticed
Brian's review comments for "[PATCH v13 02/10] xfs: Add delay ready attr
remove routines" suggesting some more code refactoring work.

No worries, thats reasonable.  It's why I only send this out in subsets 
to try and keep people sort of focused on a smaller area because stuff 
at the end of the set changes more often as a result of things moving 
around at the bottom of the set.  It doesn't make sense to channel too 
much effort into something that's still moving around so much :-)

Allison


I /think/ the defer ops will rejoin the buffer each time it rolls, which
means that xfs_attr_trans_roll returns with the buffer already joined to
the transaction?  And I think you're right that the bhold isn't needed,
because holding is dictated by the lower levels (i.e. _set_iter).

Please let me know if I am missing something obvious here.

The entire function goes away by the end of the series. :)

--D














