On Thu, Feb 25, 2021 at 07:54:01PM +0100, Christoph Hellwig wrote:

On Wed, Feb 24, 2021 at 05:34:57PM +1100, Dave Chinner wrote:

quoted

From: Dave Chinner <redacted>

Handle writing of a logvec chain into an iclog that doesn't have
enough space to fit it all. The iclog has already been changed to
WANT_SYNC by xlog_get_iclog_space(), so the entire remaining space
in the iclog is exclusively owned by this logvec chain.

The difference between the single and partial cases is that
we end up with partial iovec writes in the iclog and have to split
a log vec regions across two iclogs. The state handling for this is
currently awful and so we're building up the pieces needed to
handle this more cleanly one at a time.

I did not fully grasp the refactoring yet, so just some superficial
ramblings for now:

quoted

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 456ab3294621..74a1dddf1c15 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c

@@ -2060,6 +2060,7 @@ xlog_state_finish_copy(
 
 	be32_add_cpu(&iclog->ic_header.h_num_logops, record_cnt);
 	iclog->ic_offset += copy_bytes;
+	ASSERT(iclog->ic_offset <= iclog->ic_size);

How is this related to the rest of the patch?  Maybe just add it
in a prep patch?

Oh, it was debug code I added while tracking down loop iteration
bugs. I forgot to remove it - it didn't actually catch any bugs...

quoted

+	error = xlog_state_get_iclog_space(log, len, &iclog, ticket,
+					   &contwr, &log_offset);
+	if (error)
+		return error;
 
+	/* start_lsn is the LSN of the first iclog written to. */
+	if (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. This will always
+	 * fit in the iclog we have been already been passed.
+	 */
+	if (optype & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
+		iclog->ic_flags |= (XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FUA);
+		ASSERT(!contwr);
+	}
 
+	while (lv) {
+		lv = xlog_write_single(lv, ticket, iclog, &log_offset,
+					&len, &record_cnt, &data_cnt);
+		if (!lv)
 			break;
 
+		ASSERT(!(optype & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)));
+		lv = xlog_write_partial(log, lv, ticket, &iclog, &log_offset,
+					&len, &record_cnt, &data_cnt, &contwr);
+		if (IS_ERR(lv)) {
+			error = PTR_ERR(lv);
+			break;
 		}
 	}

Maybe user IS_ERR_OR_NULL and PTR_ERR_OR_ZERO here to catch the NULL
case as well?  e.g.

	for (;;) {
		....
		lv = xlog_write_partial();
		if (IS_ERR_OR_NULL(lv)) {
			error = PTR_ERR_OR_ZERO(lv);
			break;
		}
	}

Sure.

quoted

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index acf20c2e5018..c978c52e7ba8 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c

@@ -896,7 +896,6 @@ xlog_cil_push_work(
 	num_iovecs += lvhdr.lv_niovecs;
 	num_bytes += lvhdr.lv_bytes;
 
-
 	/*

This seems misplaced.

Yeah, another bad debug cleanup.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com