Re: [PATCH 4/8] xfs: pass a CIL context to xlog_write()

On Thu, Jun 17, 2021 at 01:24:02PM -0700, Darrick J. Wong wrote:

On Thu, Jun 17, 2021 at 06:26:13PM +1000, Dave Chinner wrote:

quoted

From: Dave Chinner <redacted>

Pass the CIL context to xlog_write() rather than a pointer to a LSN
variable. Only the CIL checkpoint calls to xlog_write() need to know
about the start LSN of the writes, so rework xlog_write to directly
write the LSNs into the CIL context structure.

This removes the commit_lsn variable from xlog_cil_push_work(), so
now we only have to issue the commit record ordering wakeup from
there.

Signed-off-by: Dave Chinner <redacted>
---
 fs/xfs/xfs_log.c      | 22 +++++++++++++++++-----
 fs/xfs/xfs_log_cil.c  | 19 ++++++++-----------
 fs/xfs/xfs_log_priv.h |  4 ++--
 3 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index cf661c155786..fc0e43c57683 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c

@@ -871,7 +871,7 @@ xlog_write_unmount_record(
 	 */
 	if (log->l_targ != log->l_mp->m_ddev_targp)
 		blkdev_issue_flush(log->l_targ->bt_bdev);
-	return xlog_write(log, &lv_chain, ticket, NULL, NULL, reg.i_len);
+	return xlog_write(log, NULL, &lv_chain, ticket, NULL, reg.i_len);
 }
 
 /*

@@ -2383,9 +2383,9 @@ xlog_write_partial(
 int
 xlog_write(
 	struct xlog		*log,
+	struct xfs_cil_ctx	*ctx,
 	struct list_head	*lv_chain,
 	struct xlog_ticket	*ticket,
-	xfs_lsn_t		*start_lsn,
 	struct xlog_in_core	**commit_iclog,
 	uint32_t		len)
 {

@@ -2408,9 +2408,21 @@ xlog_write(
 	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);
+	/*
+	 * If we have a CIL context, record the LSN of the iclog we were just
+	 * granted space to start writing into. If the context doesn't have
+	 * a start_lsn recorded, then this iclog will contain the start record
+	 * for the checkpoint. Otherwise this write contains the commit record
+	 * for the checkpoint.
+	 */
+	if (ctx) {
+		spin_lock(&ctx->cil->xc_push_lock);
+		if (!ctx->start_lsn)
+			ctx->start_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
+		else
+			ctx->commit_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
+		spin_unlock(&ctx->cil->xc_push_lock);

This cycling of the push lock when setting start_lsn is new.  What are
we protecting against here by taking the lock?

Later in the series it will be the ordering wakeup when we set the
start_lsn. The ordering ends with both start_lsn and commit_lsn
being treated the same way w.r.t. wakeups, so I just started it off
the same way here.

Also, just to check my assumptions: why do we take the push lock when
setting commit_lsn?  Is that to synchronize with the xc_committing loop
that looks for contexts that need pushing?

Yes - the spinlock provides the memory barriers for access to the
variable. I could use WRITE_ONCE/READ_ONCE here for this specific patch,
but the lock is necessary for compound operations in upcoming
patches so it didn't make any sense to use _ONCE macros here only to
remove them again later.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com