On Thu, Feb 25, 2021 at 09:58:42AM +0100, Christoph Hellwig wrote:

quoted

As a result:

	logbsize	fsmark create rate	rm -rf
before	32kb		152851+/-5.3e+04	5m28s
patched	32kb		221533+/-1.1e+04	5m24s

before	256kb		220239+/-6.2e+03	4m58s
patched	256kb		228286+/-9.2e+03	5m06s

The rm -rf times are included because I ran them, but the
differences are largely noise. This workload is largely metadata
read IO latency bound and the changes to the journal cache flushing
doesn't really make any noticable difference to behaviour apart from
a reduction in noiclog events from background CIL pushing.

The 256b rm -rf case actually seems like a regression not in the noise
here.  Does this reproduce over multiple runs?

It's noise. The unlink repeat times on this machine at 16 threads
are at least +/-15s because the removals are not synchronised in
groups like the creates are.

These are CPU bound workloads when the log is not limiting the
transaction rate (only the {before, 32kB} numbers in this test are
log IO bound) so there's always some variation in performance due to
non-deterministic factors like memory reclaim, AG lock-stepping
between threads, etc.

Hence there's a bit of unfairness between the threads and often the
first thread finishes 30s before the last thread. The times are for
the last thread completing and there can be significant variation on
that.

quoted

@@ -2009,13 +2010,14 @@ xlog_sync(
 	 * synchronously here; for an internal log we can simply use the block
 	 * layer state machine for preflushes.
 	 */
-	if (log->l_targ != log->l_mp->m_ddev_targp || split) {
+	if (log->l_targ != log->l_mp->m_ddev_targp ||
+	    (split && (iclog->ic_flags & XLOG_ICL_NEED_FLUSH))) {
 		xfs_flush_bdev(log->l_mp->m_ddev_targp->bt_bdev);
-		need_flush = false;
+		iclog->ic_flags &= ~XLOG_ICL_NEED_FLUSH;

Once you touch all the buffer flags anyway we should optimize the
log wraparound case here - insteaad of th synchronous flush we just
need to set REQ_PREFLUSH on the first log bio, which should be nicely
doable with your infrastruture.

That sounds like another patch because it's a change of behaviour.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com