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 >
