Re: [PATCH 1/9] xfs: don't set XFS_TRANS_HAS_INTENT_DONE when there's no ATTRD log item

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Dec 04, 2023 at 06:08:03AM +0100, Christoph Hellwig wrote:
> On Sun, Dec 03, 2023 at 11:02:57AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > XFS_TRANS_HAS_INTENT_DONE is a flag to the CIL that we've added a log
> > intent done item to the transaction.  This enables an optimization
> > wherein we avoid writing out log intent and log intent done items if
> > they would have ended up in the same checkpoint.  This reduces writes to
> > the ondisk log and speeds up recovery as a result.
> > 
> > However, callers can use the defer ops machinery to modify xattrs
> > without using the log items.  In this situation, there won't be an
> > intent done item, so we do not need to set the flag.
> 
> Understanding the logged attrs code is till on my TODO list, but
> the patch looks obviously correct in that we shouldn't set
> XFS_TRANS_HAS_INTENT_DONE if there is no done items.  I'm still
> confused how it can log an intent item without a done item,
> though.

Dave and Allison and I at some point realized that the defer ops
machinery works even if ->create_intent and ->create_done return NULL.
You'd lose the ability to restart the operation after a crash, but if
the upper layers can tolerate a half-finished operation
(e.g.  ATTR_INCOMPLETE) then that should be ok.

Obviously you wouldn't touch any such *existing* code except as part of
adapting it to be capable of using log items, and that's exactly what
Allison did.  She refactor the old xattr code to track the state of the
operation explicitly, then moved all that into the ->finish_item
implementation.  Now, if the setattr operation does not set the LOGGED
flag (the default), the behavior should be exactly the same as before.
If they do set LOGGED (either because the debug knob is set; or because
the caller is parent pointers) then ->create_{intent,done} actually
create log intent and done items.

It should never create an intent item and not the done item or the other
way 'round, obviously.  Either both functions return NULL, or they both
return non-NULL.

--D

> Cautiously and superficially:

Thanks! :)

> Reviewed-by: Christoph Hellwig <hch@lst.de>
>