On Tue, Mar 09, 2021 at 12:13:52PM +1100, Dave Chinner wrote:

On Mon, Mar 08, 2021 at 07:25:26AM -0500, Brian Foster wrote:

quoted

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

quoted

From: Dave Chinner <redacted>

Currently every journal IO is issued as REQ_PREFLUSH | REQ_FUA to
guarantee the ordering requirements the journal has w.r.t. metadata
writeback. THe two ordering constraints are:

....

quoted

quoted

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.

Signed-off-by: Dave Chinner <redacted>
---

Thoughts on my previous feedback to this patch, particularly the locking
bits..? I thought I saw a subsequent patch somewhere that increased the
parallelism of this code..

I seem to have missed that email, too.

Seems this occurs more frequently than it should. :/ Mailer problems?

I guess you are refering to these two hunks:

Yes.

quoted

quoted

@@ -2416,10 +2408,21 @@ xlog_write(
 		ASSERT(log_offset <= iclog->ic_size - 1);
 		ptr = iclog->ic_datap + log_offset;
 
-		/* start_lsn is the first lsn written to. That's all we need. */
+		/* Start_lsn is the first lsn written to. */
 		if (start_lsn && !*start_lsn)
 			*start_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
 
+		/*
+		 * iclogs containing commit records or unmount records need
+		 * to issue ordering cache flushes and commit immediately
+		 * to stable storage to guarantee journal vs metadata ordering
+		 * is correctly maintained in the storage media.
+		 */
+		if (optype & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
+			iclog->ic_flags |= (XLOG_ICL_NEED_FLUSH |
+						XLOG_ICL_NEED_FUA);
+		}
+
 		/*
 		 * This loop writes out as many regions as can fit in the amount
 		 * of space which was allocated by xlog_state_get_iclog_space().

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index c04d5d37a3a2..263c8d907221 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c

@@ -896,11 +896,16 @@ xlog_cil_push_work(
 
 	/*
 	 * If the checkpoint spans multiple iclogs, wait for all previous
-	 * iclogs to complete before we submit the commit_iclog.
+	 * iclogs to complete before we submit the commit_iclog. If it is in the
+	 * same iclog as the start of the checkpoint, then we can skip the iclog
+	 * cache flush because there are no other iclogs we need to order
+	 * against.
 	 */
 	if (ctx->start_lsn != commit_lsn) {
 		spin_lock(&log->l_icloglock);
 		xlog_wait_on_iclog(commit_iclog->ic_prev);
+	} else {
+		commit_iclog->ic_flags &= ~XLOG_ICL_NEED_FLUSH;
 	}

.... that set/clear the flags on the iclog?  Yes, they probably
should be atomic.

On second thoughts, we can't just clear XLOG_ICL_NEED_FLUSH here
because there may be multiple commit records on this iclog and a
previous one might require the flush. I'll just remove this
optimisation from the patch right now, because it's more complex
than it initially seemed.

Ok.

And looking at the aggregated code that I have now (including the
stuff I haven't sent out), the need for xlog_write() to set the
flush flags on the iclog is gone. THis is because the unmount record
flushes the iclog directly itself so it can add flags there, and
the iclog that the commit record is written to is returned to the
caller.

Ok.

Brian

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com