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@xxxxxxxxxxxxx
