On 9/20/19 6:49 AM, Brian Foster wrote:
On Thu, Sep 05, 2019 at 03:18: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. 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 | 43 +++++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 14 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 7a6dd37..f27e2c6 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -593,19 +593,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(
+ struct xfs_da_args *args,
+ struct xfs_buf *bp)
{
- struct xfs_buf *bp;
- int retval, error, forkoff;
- struct xfs_inode *dp = args->dp;
+ int retval, error;
It looks like we could pick either retval or error and use it
consistently throughout the new function.
trace_xfs_attr_leaf_addname(args);
I also wonder if this tracepoint should remain in the caller.
Alrihty, will clean out retval and move trace point
@@ -650,13 +643,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.
*/
error = xfs_attr3_leaf_to_node(args);
if (error)
return error;
+ }
+ return retval;
+}
+
+
+/*
+ * 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 retval, error, forkoff;
+ struct xfs_buf *bp = NULL;
+ struct xfs_inode *dp = args->dp;
+
+ retval = xfs_attr_leaf_try_add(args, bp);
+ if (retval == -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;
Hmm.. I find this bit of factoring a little strange. We do part of the
-ENOSPC handling (leaf to node) in one place and another part
(xfs_defer_finish()) in the caller. I'm assuming we intentionally don't
finish dfops in the new helper because the delayed attr bits shouldn't
do that, but I'm wondering whether the helper should just return -ENOSPC
and the caller should be responsible for whatever needs to happen based
on that in the associated context. Hm?
Brian
Yes, the idea is that the new helper function avoids anything with
transactions. We could factor up the ENOSPC handling into the caller,
that may look cleaner. I think there's really only one place it's called.
Thanks!
Allison
--
2.7.4
