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

From: Brian Foster <bfoster@redhat.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 00/19] xfs: refactor log recovery
Date: Wed, 22 Apr 2020 12:18:54 -0400	[thread overview]
Message-ID: <20200422161854.GB37352@bfoster> (raw)
In-Reply-To: <158752116283.2140829.12265815455525398097.stgit@magnolia>

On Tue, Apr 21, 2020 at 07:06:03PM -0700, Darrick J. Wong wrote:
> Hi all,
> 
> This series refactors log recovery by moving recovery code for each log
> item type into the source code for the rest of that log item type and
> using dispatch function pointers to virtualize the interactions.  This
> dramatically reduces the amount of code in xfs_log_recover.c and
> increases cohesion throughout the log code.
> 

So I realized pretty quickly after looking at patch 2 that I probably
need to step back and look at the big picture before getting into the
details of the patches. After skimming through the end result, my
preliminary reaction is that the concept looks interesting for intents,
but I'm not so sure about abstracting so aggressively across the core
recovery implementation simply because I don't think the current design
lends itself to it. Some high level thoughts on particular areas...

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

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

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

Given all that, ISTM that the virtualization pattern is perhaps best
suited for the intent bits since we'll probably grow more
xlog_recover_intent_type users over time as we defer-enable more
operations, and the interfaces therein are more broadly used across the
types. I'm much more on the fence about the whole xlog_recover_item_type
thing. I've no objection to lifting more type-specific code out of
xfs_log_recover.c, but if we're going as far as creating a virt
interface then I'd probably rather see us refactor things more first and
design a cleaner interface rather than simply spread the order,
readahead, pass1, pass2 recovery sequence logic around through multiple
source files, particularly where several of the virt interfaces are used
relatively sparsely (and the landing spots for that code is shared with
other functional components...)

Which also makes me wonder if the _item.c files are the right place to
dump functional recovery code. I've always considered those files mostly
related to xfs_log_item management so it looks a little off to see some
of the functional recovery code in there. Perhaps we should split log
recovery into its own set of per-type source files and leave any common
log item/format bits in the _item.c files..? That certainly might make
more sense to me if we do go with such a low level recovery interface...

Brian

> If you're going to start using this mess, you probably ought to just
> pull from my git trees, which are linked below.
> 
> This is an extraordinary way to destroy everything.  Enjoy!
> Comments and questions are, as always, welcome.
> 
> --D
> 
> kernel git tree:
> https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=refactor-log-recovery
> 