On Mon, Mar 08, 2021 at 06:21:34PM -0800, Darrick J. Wong wrote: > On Fri, Mar 05, 2021 at 04:11:23PM +1100, Dave Chinner wrote: > > From: Dave Chinner <dchinner@xxxxxxxxxx> > > > > Current xlog_write() adds op headers to the log manually for every > > log item region that is in the vector passed to it. While > > xlog_write() needs to stamp the transaction ID into the ophdr, we > > already know it's length, flags, clientid, etc at CIL commit time. > > > > This means the only time that xlog write really needs to format and > > reserve space for a new ophdr is when a region is split across two > > iclogs. Adding the opheader and accounting for it as part of the > > normal formatted item region means we simplify the accounting > > of space used by a transaction and we don't have to special case > > reserving of space in for the ophdrs in xlog_write(). It also means > > we can largely initialise the ophdr in transaction commit instead > > of xlog_write, making the xlog_write formatting inner loop much > > tighter. > > > > xlog_prepare_iovec() is now too large to stay as an inline function, > > so we move it out of line and into xfs_log.c. > > > > Object sizes: > > text data bss dec hex filename > > 1125934 305951 484 1432369 15db31 fs/xfs/built-in.a.before > > 1123360 305951 484 1429795 15d123 fs/xfs/built-in.a.after > > > > So the code is a roughly 2.5kB smaller with xlog_prepare_iovec() now > > out of line, even though it grew in size itself. > > > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx> > > Sooo... if I understand this part of the patchset correctly, the goal > here is to simplify and shorten the inner loop of xlog_write. That's one of the goals. The other goal is to avoid needing to account for log op headers separately in the high level CIL commit code. > Callers > are now required to create their own log op headers at the start of the > xfs_log_iovec chain in the xfs_log_vec, which means that the only time > xlog_write has to create an ophdr is when we fill up the current iclog > and must continue in a new one, because that's not something the callers > should ever have to know about. Correct? Yes. > If so, > Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx> Thanks! > It /really/ would have been nice to have kept these patches separated by > major functional change area (i.e. separate series) instead of one > gigantic 45-patch behemoth to intimidate the reviewers... How is that any different from sending out 6-7 separate dependent patchsets one immediately after another? A change to one patch in one series results in needing to rebase at least one patch in each of the smaller patchsets, so I've still got to treat them all as one big patchset in my development trees. Then I have to start reposting patchsets just because another patchset was changed, and that gets even more confusing trying to work out what patchset goes with which version and so on. It's much easier for me to manage them as a single patchset.... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
