Re: [PATCH 00/19] xfs: refactor log recovery

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Apr 28, 2020 at 08:43:42AM -0400, Brian Foster wrote:
> On Mon, Apr 27, 2020 at 11:12:08PM -0700, Christoph Hellwig wrote:
> > On Wed, Apr 22, 2020 at 12:18:54PM -0400, Brian Foster wrote:
> > > - Transaction reorder
> > > 
> > > Virtualizing the transaction reorder across all several files/types
> > > strikes me as overkill for several reasons. From a code standpoint,
> > > we've created a new type enumeration and a couple fields (enum type and
> > > a function) in a generic structure to essentially abstract out the
> > > buffer handling into a function. The latter checks another couple of blf
> > > flags, which appears to be the only real "type specific" logic in the
> > > whole sequence. From a complexity standpoint, the reorder operation is a
> > > fairly low level and internal recovery operation. We have this huge
> > > comment just to explain exactly what's happening and why certain items
> > > have to be ordered as such, or some treated like others, etc. TBH it's
> > > not terribly clear even with that documentation, so I don't know that
> > > splitting the associated mapping logic off into separate files is
> > > helpful.
> > 
> > I actually very much like idea of moving any knowledge of the individual
> > item types out of xfs_log_recovery.c.  In reply to the patch I've
> > suggsted an idea how to kill the knowledge for all but the buffer and
> > icreate items, which should make this a little more sensible.
> > 
> 
> I mentioned to Darrick the other day briefly on IRC that I don't
> fundamentally object to splitting up xfs_log_recover.c. I just think
> this mechanical split out of the existing code includes too much of the
> implementation details of recovery and perhaps abstracts a bit too much.
> I find the general idea much more acceptable with preliminary cleanups
> and a more simple interface.

It's cleaned up considerably with hch's cleanup patches 1-5 of 2. ;)

> > I actually think we should go further in one aspect - instead of having
> > the item type to ops mapping in a single function in xfs_log_recovery.c
> > we should have a table that the items can just add themselves to.
> > 
> 
> That sounds reasonable, but that's more about abstraction mechanism than
> defining the interface. I was more focused on simplifying the latter in
> my previous comments.

<nod>

> > > - Readahead
> > > 
> > > We end up with readahead callouts for only the types that translate to
> > > buffers (so buffers, inode, dquots), and then those callouts do some
> > > type specific mapping (that is duplicated within the specific type
> > > handers) and issue a readahead (which is duplicated across each ra_pass2
> > > call). I wonder if this would be better abstracted by a ->bmap() like
> > > call that simply maps the item to a [block,length] and returns a
> > > non-zero length if the core recovery code should invoke readahead (after
> > > checking for cancellation). It looks like the underlying implementation
> > > of those bmap calls could be further factored into helpers that
> > > translate from the raw record data into the type specific format
> > > structures, and that could reduce duplication between the readahead
> > > calls and the pass2 calls in a couple cases. (The more I think about,
> > > the more I think we should introduce those kind of cleanups before
> > > getting into the need for function pointers.)
> > 
> > That sounds more complicated what we have right now, and even more so
> > with my little xlog_buf_readahead helper.  Yes, the methods will all
> > just call xlog_buf_readahead, but they are trivial two-liners that are
> > easy to understand.  Much easier than a complicated calling convention
> > to pass the blkno, len and buf ops back.
> > 
> 
> Ok. The above was just an idea to simplify things vs. duplicating
> readahead code and recovery logic N times. I haven't seen your
> idea/code, but if that problem is addressed with a helper vs. a
> different interface then that seems just as reasonable to me.
> 
> > > - Recovery (pass1/pass2)
> > > 
> > > The core recovery bits seem more reasonable to factor out in general.
> > > That said, we only have two pass1 callbacks (buffers and quotaoff). The
> > > buffer callback does cancellation management and the quotaoff sets some
> > > flags, so I wonder why those couldn't just remain as direct function
> > > calls (even if we move the functions out of xfs_log_recover.c). There
> > > are more callbacks for pass2 so the function pointers make a bit more
> > > sense there, but at the same time it looks like the various intents are
> > > further abstracted behind a single "intent type" pass2 call (which has a
> > > hardcoded XLOG_REORDER_INODE_LIST reorder value and is about as clear as
> > > mud in that context, getting to my earlier point).
> > 
> > Again I actually like the callouts, mostly because they make it pretty
> > clear what is going on.  I also really like the fact that the recovery
> > code is close to the code actually writing the log items.

Looking back at that, I realize that (provided nobody minds having
function dispatch structures that are sort of sparse) there's no reason
why we need to have separate xlog_recover_intent_type and
xlog_recover_item_type structures.

> I find both the runtime logging and recovery code to be complex enough
> individually that I prefer not to stuff them together, but there is
> already precedent with dfops and such so that's not the biggest deal to
> me if the interface is simplified (and hopefully amount of code
> reduced).

I combined them largely on the observation that with the exception of
buffers, log item recovery code is generally short and not worth
creating even more files.  224 is enough.

--D

> Brian
>