On 11/8/19 1:57 PM, Darrick J. Wong wrote:
On Wed, Nov 06, 2019 at 06:27:52PM -0700, Allison Collins wrote:
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. This will help
to reduce repetitive code later when we introduce
delayed attributes.
Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx>
---
fs/xfs/libxfs/xfs_attr.c | 84 +++++++++++++++++++++++++++++-------------------
1 file changed, 51 insertions(+), 33 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 212995f..dda2eba 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -305,10 +305,33 @@ 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
+ if (error == -ENOSPC) {
+ /*
+ * 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;
+
+ /*
+ * Fob the rest of the problem off on the Btree code.
+ */
+ error = xfs_attr_node_addname(args);
+ }
+ } else {
error = xfs_attr_node_addname(args);
+ }
return error;
}
@@ -601,21 +624,12 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
* External routines when attribute list is one block
*========================================================================*/
-/*
- * 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)
+xfs_attr_leaf_try_add(
(total stream of consciousness here...)
AFAICT the old _addname function's responsibilities were:
1 Try to add a new attr key entry to the leaf block, with INCOMPLETE set
if it's a rename op or we need to set a remote value.
2 If there wasn't space in the leaf block, convert to node format, call
the node version of this function, and exit.
3 Allocating blocks for the remote attr value and writing them, if
applicable
4 If it's a rename operation, clearing the INCOMPLETE flag on the new
entry; setting it on the old entry; and then removing the old entry.
5 Clearing the INCOMPLETE flag on the new entry when we're done writing
a remote value (if applicable)
I think we arrive at this split so that we don't have a transaction roll
in the middle of the function, right? And also to make the "convert to
node format and roll" bits go elsewhere?
The way I'm thinking about how to accomplish this is...
xfs_attr_leaf_addname should be renamed xfs_attr_leaf_setname, and then
hoist (1) into a separate function, move (2) into xfs_attr_set_args, and
hoist (4) into a separate function.
...ok, so let's test how closely my understanding fits the changes made
in this patch:
_try_add is basically (1).
Most of (2) happened, though the call to xfs_attr3_leaf_to_node ought to
go into the caller so that the conversion stays with the defer_finish
and roll.
(4) could still be done, maybe as a separate prep patch.
I think you're on the right track. Perhaps the diff made it look a
little crazier than it seems. Maybe its easier to describe it as this:
I renamed xfs_attr_leaf_addname to xfs_attr_leaf_try_add,
and then deleted (2) out of the body of the function. The upper half
(1) became the helper function, and lower half (3, 4, and 5) became the
new xfs_attr_leaf_addname, which now calls the helper at the start of
the routine. Finally (2) got factored up into the caller.
Hope that helps some :-) The point of all this being to jigsaw out (1)
into a stand alone routine that we can use later in delayed attrs.
Hm, ok, I think I understand what this patch does. The call site in
xfs_attr_set_args would be clearer (and less indenty) if it looked like:
if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
error = xfs_attr_leaf_addname(args);
if (error == 0 || error != -ENOSPC)
return error;
/* Promote the attribute list to node format. */
error = xfs_attr3_leaf_to_node(args);
if (error)
return error;
I think this part here is already taken care of in
xfs_attr_leaf_addname, but otherwise, I think the rest of this is
equivalent. Will update in the next set.
Thanks for the reviews!
Allison
/*
* 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;
}
return xfs_attr_node_addname(args);
But otherwise it looks decent, assuming I understood any of it. :)
--D
+ 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
@@ -661,31 +675,35 @@ 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;
+}
- /*
- * 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
--
2.7.4
