On Tue, Apr 26, 2022 at 05:34:47PM -0700, Alli wrote:
> On Thu, 2022-04-14 at 19:44 +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > xfs_attri_remove_iter is not used anymore, so remove it and all the
> > infrastructure it uses and is needed to drive it.
> >
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> > - case XFS_DAS_RMTBLK:
> > - attr->xattri_dela_state = XFS_DAS_RMTBLK;
> > -
> > - /*
> > - * If there is an out-of-line value, de-allocate the
> > blocks.
> > - * This is done before we remove the attribute so that
> > we don't
> > - * overflow the maximum size of a transaction and/or
> > hit a
> > - * deadlock.
> > - */
> > - if (args->rmtblkno > 0) {
> > - /*
> > - * May return -EAGAIN. Roll and repeat until
> > all remote
> > - * blocks are removed.
> > - */
> > - error = xfs_attr_rmtval_remove(attr);
> > - if (error == -EAGAIN) {
> > - trace_xfs_attr_remove_iter_return(
> > - attr->xattri_dela_state, args-
> > >dp);
> > - return error;
> > - } else if (error) {
> > - goto out;
> > - }
> > -
> > - /*
> > - * Refill the state structure with buffers (the
> > prior
> > - * calls released our buffers) and close out
> > this
> > - * transaction before proceeding.
> > - */
> > - ASSERT(args->rmtblkno == 0);
> > - error = xfs_attr_refillstate(state);
>
> I think you can remove xfs_attr_refillstate too. I'm getting some
> compiler gripes about that being declared but not used, and I'm pretty
> sure this was the last call to it, so probably it can go too.
I'm going to ifdef it out for now, which will remove the compiler
warning. I'll leave a comment explaining why it's been left there
unused for the moment. That is a bit of a long story.....
.... so it's Story Time! :)
The xfs_attr_savestate(), xfs_attr_refillstate() pair are for saving
the state path cursor that points to the name entry in the btree so
we don't have to look it up again after we've removed the remote
value blocks.
Removing the remote value extents trashes the state cursor (points to
remote value extents that have been removed on return, not the name
entry in the btree), so to avoid doing another tree walk we save the
disk addresses of the path blocks before remote extent removal and
restore them to the state cursor afterwards.
This avoids having to walk the hash tree to find the data block that
contains the name entry again to remove that. It's an optimisation
that I removed when combining the two state machines because it hurt
my brain trying to reconcile the two paths. Hence I ended up using
the simpler _set_iter() path that just does the lookup a second time
when I combined the state machines rather than the more complex
save/restore path in the remove_iter state machine.
I left the code there because I want to re-introduce the
optimisation in future - it will benefit both the replace and remove
paths - but I want to get the code working correctly first before I
worry too much about intricate performance optimisations like this.
> Other
> than that this patch looks ok.
>
> Reviewed-by: Allison Henderson<allison.henderson@xxxxxxxxxx>
Thanks!
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
