On Tue, Feb 25, 2020 at 05:42:07PM +1100, Dave Chinner wrote:
> On Sat, Feb 22, 2020 at 07:05:59PM -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.
>
> 68-72 columns :P
>
> >
> > Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx>
> > Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_attr.c | 88 +++++++++++++++++++++++++++++++-----------------
> > 1 file changed, 57 insertions(+), 31 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index cf0cba7..b2f0780 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;
>
> return xfs_attr_node_addname(args);
>
> better yet:
>
> if (!xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> return xfs_attr_node_addname(args);
>
> error = xfs_attr_leaf_addname(args);
> if (error != -ENOSPC)
> return error;
> .....
>
> BTW, I think I see the pattern now - isolate all the metadata
> changes from the mechanism of rolling the transactions, which means
> it can be converted to a set of operations connected by a generic
> transaction rolling mechanism. It's all starting to make more sense
> now :P
>
Yeah.. IIRC the initial attempt at this was going down the path of
creating an entire separate xattr codepath for xattr intents. The
existing codepath has the issue of rolling transactions all over the
place, which makes it difficult to execute from dfops context. The goal
is then to try and take this thing apart such that it works in dfops
context for intents and at the same time can be driven by a high level
transaction rolling loop to support the traditional xattr codepath for
backwards compatibility.
The whole state/context thing fell out of that. I think most agree that
it's ugly, but it's a useful intermediate step for breaking down this
hunk of code into components that can be further evaluated for
simplification and reduction (while making functional progress as well).
E.g., perhaps the top-level states can be eliminated in favor of simply
looking at current xattr fork format. This gets hairier deeper down in
the set path, but I think we may eventually see some similar patterns
emerge when you consider that leaf and node adds follow a very similar
flow and make many of the same function calls with regard to handling
remote values, renames, etc. etc. All in all, I think it will be
interesting if we could come up with some abstractions that ultimately
facilitate removal of the state stuff. That's certainly not clear
enough to me at least right now, but something to keep in mind when
working through this code..
Brian
> > @@ -679,31 +700,36 @@ 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
> > */
>
> Comments should use all 80 columns...
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
