On Thu, 2022-04-14 at 19:44 +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@xxxxxxxxxx> > > We currently set it and hold it when converting from short to leaf > form, then release it only to immediately look it back up again > to do the leaf insert. > > Do a bit of refactoring to xfs_attr_leaf_try_add() to avoid this > messy handling of the newly allocated leaf buffer. > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx> Ok, I think that looks good Reviewed-by: Allison Henderson<allison.henderson@xxxxxxxxxx> > --- > fs/xfs/libxfs/xfs_attr.c | 50 +++++++++++++++++++++++++------------- > -- > 1 file changed, 32 insertions(+), 18 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index b3d918195160..a4b0b20a3bab 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -319,7 +319,15 @@ xfs_attr_leaf_addname( > int error; > > if (xfs_attr_is_leaf(dp)) { > + > + /* > + * Use the leaf buffer we may already hold locked as a > result of > + * a sf-to-leaf conversion. The held buffer is no > longer valid > + * after this call, regardless of the result. > + */ > error = xfs_attr_leaf_try_add(args, attr- > >xattri_leaf_bp); > + attr->xattri_leaf_bp = NULL; > + > if (error == -ENOSPC) { > error = xfs_attr3_leaf_to_node(args); > if (error) > @@ -341,6 +349,8 @@ xfs_attr_leaf_addname( > } > next_state = XFS_DAS_FOUND_LBLK; > } else { > + ASSERT(!attr->xattri_leaf_bp); > + > error = xfs_attr_node_addname_find_attr(attr); > if (error) > return error; > @@ -396,12 +406,6 @@ xfs_attr_set_iter( > */ > if (xfs_attr_is_shortform(dp)) > return xfs_attr_sf_addname(attr); > - if (attr->xattri_leaf_bp != NULL) { > - xfs_trans_bhold_release(args->trans, > - attr->xattri_leaf_bp); > - attr->xattri_leaf_bp = NULL; > - } > - > return xfs_attr_leaf_addname(attr); > > case XFS_DAS_FOUND_LBLK: > @@ -992,18 +996,31 @@ xfs_attr_leaf_try_add( > struct xfs_da_args *args, > struct xfs_buf *bp) > { > - int retval; > + int error; > > /* > - * Look up the given attribute in the leaf block. Figure out > if > - * the given flags produce an error or call for an atomic > rename. > + * If the caller provided a buffer to us, it is locked and held > in > + * the transaction because it just did a shortform to leaf > conversion. > + * Hence we don't need to read it again. Otherwise read in the > leaf > + * buffer. > */ > - retval = xfs_attr_leaf_hasname(args, &bp); > - if (retval != -ENOATTR && retval != -EEXIST) > - return retval; > - if (retval == -ENOATTR && (args->attr_flags & XATTR_REPLACE)) > + if (bp) { > + xfs_trans_bhold_release(args->trans, bp); > + } else { > + error = xfs_attr3_leaf_read(args->trans, args->dp, 0, > &bp); > + if (error) > + return error; > + } > + > + /* > + * Look up the xattr name to set the insertion point for the > new xattr. > + */ > + error = xfs_attr3_leaf_lookup_int(bp, args); > + if (error != -ENOATTR && error != -EEXIST) > goto out_brelse; > - if (retval == -EEXIST) { > + if (error == -ENOATTR && (args->attr_flags & XATTR_REPLACE)) > + goto out_brelse; > + if (error == -EEXIST) { > if (args->attr_flags & XATTR_CREATE) > goto out_brelse; > > @@ -1023,14 +1040,11 @@ xfs_attr_leaf_try_add( > args->rmtvaluelen = 0; > } > > - /* > - * Add the attribute to the leaf block > - */ > return xfs_attr3_leaf_add(bp, args); > > out_brelse: > xfs_trans_brelse(args->trans, bp); > - return retval; > + return error; > } > > /*