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

[Christoph Hellwig <hch@infradead.org>]

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 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.

> - 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.

> - 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.