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

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

quoted

From: Dave Chinner <redacted>

We currently attach iclog callbacks for the CIL when the commit
iclog is returned from xlog_write. Because
xlog_state_get_iclog_space() always guarantees that the commit
record will fit in the iclog it returns, we can move this IO
callback setting to xlog_cil_set_ctx_write_state(), record the
commit iclog in the context and remove the need for the commit iclog
to be returned by xlog_write() altogether.


Signed-off-by: Dave Chinner <redacted>
---
 fs/xfs/xfs_log.c      |  8 ++----
 fs/xfs/xfs_log_cil.c  | 65 +++++++++++++++++++++++++------------------
 fs/xfs/xfs_log_priv.h |  3 +-
 3 files changed, 42 insertions(+), 34 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 1c214b395223..359246d54db7 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, NULL, &lv_chain, ticket, NULL, reg.i_len);
+	return xlog_write(log, NULL, &lv_chain, ticket, reg.i_len);
 }
 
 /*

@@ -2386,7 +2386,6 @@ xlog_write(
 	struct xfs_cil_ctx	*ctx,
 	struct list_head	*lv_chain,
 	struct xlog_ticket	*ticket,
-	struct xlog_in_core	**commit_iclog,
 	uint32_t		len)
 {
 	struct xlog_in_core	*iclog = NULL;

@@ -2436,10 +2435,7 @@ xlog_write(
 	 */
 	spin_lock(&log->l_icloglock);
 	xlog_state_finish_copy(log, iclog, record_cnt, 0);
-	if (commit_iclog)
-		*commit_iclog = iclog;
-	else
-		error = xlog_state_release_iclog(log, iclog, ticket);
+	error = xlog_state_release_iclog(log, iclog, ticket);
 	spin_unlock(&log->l_icloglock);
 
 	return error;

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 2d8d904ffb78..87e30917ce2e 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c

@@ -799,11 +799,34 @@ xlog_cil_set_ctx_write_state(
 
 	ASSERT(!ctx->commit_lsn);
 	spin_lock(&cil->xc_push_lock);
-	if (!ctx->start_lsn)
+	if (!ctx->start_lsn) {
 		ctx->start_lsn = lsn;
-	else
-		ctx->commit_lsn = lsn;
+		spin_unlock(&cil->xc_push_lock);
+		return;
+	}
+
+	/*
+	 * Take a reference to the iclog for the context so that we still hold
+	 * it when xlog_write is done and has released it. This means the
+	 * context controls when the iclog is released for IO.
+	 */
+	atomic_inc(&iclog->ic_refcnt);

Where do we drop this refcount?

In xlog_cil_push_work() where we call xlog_state_release_iclog().

Is this the accounting adjustment that
we have to make because xlog_write always decrements the iclog refcount
now?

Yes.

quoted

+	ctx->commit_iclog = iclog;
+	ctx->commit_lsn = lsn;
 	spin_unlock(&cil->xc_push_lock);

I've noticed how the setting of ctx->commit_lsn has moved to before the
point where we splice callback lists, only to move them back below in
the next patch.  That has made it harder for me to understand this
series.

I /think/ the goal of this patch is not really a functional change so
much as a refactoring to make the cil context track the commit iclog
directly and then smooth out some of the refcounting code, but the
shuffling around of these variables makes me wonder if I'm missing some
other subtlety.

The subtlety is that we can't issue the wakup on the commit_lsn
until after the callbacks are attached to the commit iclog. When we
set ctx->commit_lsn doesn't really matter - I'm trying to keep the
order of "callbacks attached before we issue the wakeup" so that
when the waiter is woken and then adds it's callbacks to the same
iclog they will be appended to the list after the first commit
record's callbacks and hence they get processed in the correct order
when journal IO completion runs the callbacks on that iclog.

This patch doesn't move the wakeup from after the xlog_write() call
completes, so the ordering of setting
ctx->commit_lsn and attaching the callbacks inside xlog_write()
doesn't really matter. In the next patch, the wakeups move inside
xlog_write()->xlog_cil_set_ctx_write_state(), and so now it has to
ensure that the ordering is correct.

I'll rework the patches so that this one sets up the order the next
patch requires rather than minimal change in this patch and reorder
in the next patch...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com