Re: [PATCH RESEND v2 01/18] xfs: Fix multi-transaction larp replay

[Dave Chinner <david@fromorbit.com>]

On Tue, Aug 09, 2022 at 10:01:49PM -0700, Alli wrote:
> On Wed, 2022-08-10 at 11:58 +1000, Dave Chinner wrote:
> > On Tue, Aug 09, 2022 at 09:52:55AM -0700, Darrick J. Wong wrote:
> > > On Thu, Aug 04, 2022 at 12:39:56PM -0700, Allison Henderson wrote:
> > > > Recent parent pointer testing has exposed a bug in the underlying
> > > > attr replay.  A multi transaction replay currently performs a
> > > > single step of the replay, then deferrs the rest if there is more
> > > > to do.
> > 
> > Yup.
> > 
> > > > This causes race conditions with other attr replays that
> > > > might be recovered before the remaining deferred work has had a
> > > > chance to finish.
> > 
> > What other attr replays are we racing against?  There can only be
> > one incomplete attr item intent/done chain per inode present in log
> > recovery, right?
> No, a rename queues up a set and remove before committing the
> transaction.  One for the new parent pointer, and another to remove the
> old one.

Ah. That really needs to be described in the commit message -
changing from "single intent chain per object" to "multiple
concurrent independent and unserialised intent chains per object" is
a pretty important design rule change...

The whole point of intents is to allow complex, multi-stage
operations on a single object to be sequenced in a tightly
controlled manner. They weren't intended to be run as concurrent
lines of modification on single items; if you need to do two
modifications on an object, the intent chain ties the two
modifications together into a single whole.

One of the reasons I rewrote the attr state machine for LARP was to
enable new multiple attr operation chains to be easily build from
the entry points the state machien provides. Parent attr rename
needs a new intent chain to be built, not run multiple independent
intent chains for each modification.

> It cant be an attr replace because technically the names are
> different.

I disagree - we have all the pieces we need in the state machine
already, we just need to define separate attr names for the
remove and insert steps in the attr intent.

That is, the "replace" operation we execute when an attr set
overwrites the value is "technically" a "replace value" operation,
but we actually implement it as a "replace entire attribute"
operation.

Without LARP, we do that overwrite in independent steps via an
intermediate INCOMPLETE state to allow two xattrs of the same name
to exist in the attr tree at the same time. IOWs, the attr value
overwrite is effectively a "set-swap-remove" operation on two
entirely independent xattrs, ensuring that if we crash we always
have either the old or new xattr visible.

With LARP, we can remove the original attr first, thereby avoiding
the need for two versions of the xattr to exist in the tree in the
first place. However, we have to do these two operations as a pair
of linked independent operations. The intent chain provides the
linking, and requires us to log the name and the value of the attr
that we are overwriting in the intent. Hence we can always recover
the modification to completion no matter where in the operation we
fail.

When it comes to a parent attr rename operation, we are effectively
doing two linked operations - remove the old attr, set the new attr
- on different attributes. Implementation wise, it is exactly the
same sequence as a "replace value" operation, except for the fact
that the new attr we add has a different name.

Hence the only real difference between the existing "attr replace"
and the intent chain we need for "parent attr rename" is that we
have to log two attr names instead of one. Basically, we have a new
XFS_ATTRI_OP_FLAGS... type for this, and that's what tells us that
we are operating on two different attributes instead of just one.

The recovery operation becomes slightly different - we have to run a
remove on the old, then a replace on the new - so there a little bit
of new code needed to manage that in the state machine.

These, however, are just small tweaks on the existing replace attr
operation, and there should be little difference in performance or
overhead between a "replace value" and a "replace entire xattr"
operation as they are largely the same runtime operation for LARP.

> So the recovered set grows the leaf, and returns the egain, then rest
> gets capture committed.  Next up is the recovered remove which pulls
> out the fork, which causes problems when the rest of the set operation
> resumes as a deferred operation.

Yup, and all this goes away when we build the right intent chain for
replacing a parent attr rename....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com