On 1/21/20 4:01 PM, Darrick J. Wong wrote:
On Sat, Jan 18, 2020 at 03:50:25PM -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, and move the commit into
the calling function.
Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx>
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
---
fs/xfs/libxfs/xfs_attr.c | 83 ++++++++++++++++++++++++++++--------------------
1 file changed, 49 insertions(+), 34 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index b0ec25b..9ed7e94 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;
}
@@ -607,21 +627,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).
- */
These functions are complicated enough as they are now, please make sure
they all have comments laying out the expected input metadata states and
what output states result from them.
I /think/ this function takes an inode whose attr data is in leaf
format, and tries to add a new entry. If that succeeds then we exit
with dirty inode and transaction. If ENOSPC then we convert the attr
data to node format and exit w/ dirty inode + transaction, presuming
that the caller will try again with xfs_attr_node_addname?
That sounds about right. How about the following commentary then:
/
/*
* Tries to add an attribute to an inode in leaf form
*
* 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.
*/
Sound good?
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
@@ -667,31 +678,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)
STATIC int
xfs_attr_leaf_addname(
struct xfs_da_args *args)
{
Please fix the inconsistent style things as you touch them.
--D
Sure, will do.
Allison
+{
+ 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
