On Thu, Mar 12, 2021 at 02:29:32PM +1100, Dave Chinner wrote:

On Mon, Mar 08, 2021 at 06:21:34PM -0800, Darrick J. Wong wrote:

quoted

On Fri, Mar 05, 2021 at 04:11:23PM +1100, Dave Chinner wrote:

quoted

From: Dave Chinner <redacted>

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

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.

quoted

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.

quoted

If so,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

Thanks!

quoted

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

Well, ok, but it would have been nice for the cover letter to give
/some/ hint as to what's changing in various subranges, e.g.

"Patches 32-36 reduce the xc_cil_lock critical sections,
 Patches 37-41 create per-cpu cil structures and move log items and
       vectors to use them,
 Patches 42-44 are more cleanups,
 Patch 45 documents the whole mess."

So I could see the outlines of where the 45 patches were going.

--D

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com