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.
