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 >
