[00/10] xfs: LARP - clean up xfs_attr_set_iter state machine

Message

Dave Chinner April 12, 2022, 4:25 a.m. UTC

Hi Allison,

This is first patchset for fixing up stuff in the LARP code. I've
based this on my current 5.19-compose branch here:

git://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git xfs-5.19-compose

The first patch in the series fixes the splat that occurs in
generic/642 in that merge from the empty, dirty transaction. I
haven't touched the xfs_attri_finish_one() code to remove the
XFS_TRANS_DIRTY there because that code is used for the remove path,
too, and I didn't want to perturb that before I was finished with
the set path.

The rest of the patchset is cleaning up the xfs_attr_set_iter()
state machine. THe use of XFS_DAS_UNINIT is gone - instead I set the
initial state according to the format of the attr fork. Then if we
convert from sf to leaf, or leaf to node, we bump the state to
LEAF_ADD or NODE_ADD and roll the transaction. The next time in it
will perform the appropriate attr addition.

I've then added extra states to handle remote value block allocation
and setting of the value for the leaf blocks. This makes the code
the same as setting the remote value for node blocks, and that then
leads to collapsing all the duplicate code paths.

To do that, I set up the leaf and node states as identical
numerically ascending sequences, allowing state changes to be done
by incrementing the state value from a specific initial condition,
but progressing down the correct sequence of states even though they
are executing the same code path. This initial condition (leaf or
node) is set directly by the LEAF/NODE_ADD states that have already
been separated and set up.

From there, it's really just cleanup - I separated the flipflags
operation into a separate state, so when larp is enabled it just
skips straight over it. I renamed the states to describe the
high level operation it is performing rather than the mechanics of
the modification it is making. Like the remote val block alloc, I
enabled it to skip over the remote attr states on remove if it isn't
a remote attr.

I factored the code a bit more, cleaning up the final leaf/node
states to look the same from the perspective of the state machine.

I then made sure that the state machine has a termination state -
XFS_DAS_DONE - so taht callers can determine whether the operation
is complete without requiring xfs_attr_set_iter() to return -EAGAIN
to tell the caller it needs to keep iterating. This allows removal
of most of the -EAGAIN returns and conditional logic in the
xfs_attr_set_iter() implementation and leaf functions. This means
that requirement of the deferop transaction rolling API (return
-EAGAIN) is contained entirely within xfs_attri_finish_one() instead
of bleeding through to xfs_attr_set_iter().

Overall, I find the state machine code much easier to read and
follow because it largely separates the implementation of individual
states from the execution of the state machine. The states are
smaller and easier to understate, too.

What I haven't done yet is update the big flowchart in xfs_attr.h.
Much of it is now stale and it doesn't match the new states or
progression through the states. I'm wondering if there's a better
way to document the state machine than a comment that will get
rapidly out of date, even though that comment was very helpful in
working out how to re-write the state machine cleanly....

I plan to make the same structural mods to xfs_attr_remove_iter(),
and then I can clean up xfs_attri_finish_one() and get rid of the
XFS_TRANS_DIRTY in that code.

The diffstat isn't too bad - it doesn't make the code smaller
overall because I split a lot of stuff out into smaller functions,
but it doesn't get bigger, either, and I think the result is much
more readable and maintainable.

 fs/xfs/libxfs/xfs_attr.c | 586 ++++++++++++++++-------------------
 fs/xfs/libxfs/xfs_attr.h |  60 +++-
 fs/xfs/xfs_attr_item.c   |   2 +
 fs/xfs/xfs_trace.h       |  26 +-
 4 files changed, 345 insertions(+), 329 deletions(-)

The patchset passes fstests '-g attr' running in a loop when larp=0,
but I haven't tested it with larp=1 yet - I've done zero larp=1
testing so far so I don't even know whether it works in the base
5.19 compose yet. I'll look at that when I finish the state machine
updates....

Thoughts?

Cheers,

Dave.

Comments

Christoph Hellwig April 12, 2022, 4:48 a.m. UTC | #1

On Tue, Apr 12, 2022 at 02:25:33PM +1000, Dave Chinner wrote:
> Hi Allison,
> 
> This is first patchset for fixing up stuff in the LARP code. I've
> based this on my current 5.19-compose branch here:

What is "LARP"?

Dave Chinner April 12, 2022, 5:04 a.m. UTC | #2

On Mon, Apr 11, 2022 at 09:48:04PM -0700, Christoph Hellwig wrote:
> On Tue, Apr 12, 2022 at 02:25:33PM +1000, Dave Chinner wrote:
> > Hi Allison,
> > 
> > This is first patchset for fixing up stuff in the LARP code. I've
> > based this on my current 5.19-compose branch here:
> 
> What is "LARP"?

Logged Attribute RePlay.

https://lore.kernel.org/linux-xfs/[e-mail]/

Cheers,

Dave.

Dave Chinner April 12, 2022, 10:42 a.m. UTC | #3

On Tue, Apr 12, 2022 at 02:25:33PM +1000, Dave Chinner wrote:
> Hi Allison,
> 
> This is first patchset for fixing up stuff in the LARP code. I've
....

> The patchset passes fstests '-g attr' running in a loop when larp=0,
> but I haven't tested it with larp=1 yet - I've done zero larp=1
> testing so far so I don't even know whether it works in the base
> 5.19 compose yet. I'll look at that when I finish the state machine
> updates....

With patch 11, larp=1 passes all but generic/642 - I screwed up a
state change that affects the larp=1 mode, so there's small changes to
patch 7 and rebasing for 8-10 as a result. Overall the code
structure doesn't change, just the transition to REPLACE/REMOVE_OLD
states.

I'm testing the updated series now - it seems like it is working in
both larp=0 and larp=1 mode. I'll let it run overnight and go from
them.

Cheers,

Dave.

Allison Henderson April 12, 2022, 5:28 p.m. UTC | #4

On Tue, 2022-04-12 at 20:42 +1000, Dave Chinner wrote:
> On Tue, Apr 12, 2022 at 02:25:33PM +1000, Dave Chinner wrote:
> > Hi Allison,
> > 
> > This is first patchset for fixing up stuff in the LARP code. I've
> ....
> 
> > The patchset passes fstests '-g attr' running in a loop when
> > larp=0,
> > but I haven't tested it with larp=1 yet - I've done zero larp=1
> > testing so far so I don't even know whether it works in the base
> > 5.19 compose yet. I'll look at that when I finish the state machine
> > updates....
> 
> With patch 11, larp=1 passes all but generic/642 - I screwed up a
> state change that affects the larp=1 mode, so there's small changes
> to
> patch 7 and rebasing for 8-10 as a result. Overall the code
> structure doesn't change, just the transition to REPLACE/REMOVE_OLD
> states.
> 
> I'm testing the updated series now - it seems like it is working in
> both larp=0 and larp=1 mode. I'll let it run overnight and go from
> them.
> 
> Cheers,
> 
> Dave.

Alrighty, I'll take a look at what you have so far and will wait until
I hear from you on this. Then we can run it through the delayed attr
tests.  If I can get pptrs on it as well, it's pretty good about
finding any bugs in the underlying attribute mechanics too.

Thanks!
Allison

>