On Mon, Jul 05, 2021 at 03:49:19PM +1000, Dave Chinner wrote:

On Fri, Jul 02, 2021 at 10:17:57AM +0100, Christoph Hellwig wrote:

quoted

On Wed, Jun 30, 2021 at 05:21:07PM +1000, Dave Chinner wrote:

quoted

From: Dave Chinner <redacted>

Now that we have a mechanism to guarantee that the callbacks
attached to an iclog are owned by the context that attaches them
until they drop their reference to the iclog via
xlog_state_release_iclog(), we can attach callbacks to the iclog at
any time we have an active reference to the iclog.

xlog_state_get_iclog_space() always guarantees that the commit
record will fit in the iclog it returns, so 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.

This, in turn, allows us to move the wakeup for ordered commit
recrod writes up into xlog_cil_set_ctx_write_state(), too, because

s/recrod/record/

quoted

--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c

@@ -646,11 +646,41 @@ xlog_cil_set_ctx_write_state(
 	xfs_lsn_t		lsn = be64_to_cpu(iclog->ic_header.h_lsn);
 
 	ASSERT(!ctx->commit_lsn);
+	if (!ctx->start_lsn) {
+		spin_lock(&cil->xc_push_lock);
 		ctx->start_lsn = lsn;
+		spin_unlock(&cil->xc_push_lock);
+		return;

What does xc_push_lock protect here?  None of the read of
->start_lsn are under xc_push_lock, and this patch moves one of the
two readers to be under l_icloglock.

For this patch - nothing. It just maintains the consistency
introduced in the previous patch of doing the CIL context updates
under the xc_push_lock. I did that in the previous patch for
simplicity: the next patch adds the start record ordering which,
like the commit record ordering, needs to set ctx->start_lsn and run
the waiter wakeup under the xc_push_lock.

quoted

Also I wonder if the comment about what is done if start_lsn is not
set would be better right above the if instead of on top of the function
so that it stays closer to the code it documents.

I think it's better to document calling conventions at the top of
the function, rather than having to read the implementation of the
function to determine how it is supposed to be called. i.e. we
expect two calls to this function per CIL checkpoint - the first for
the start record ordering, the second for the commit record
ordering...

For calling conventions, I totally agree.  It's a lot easier to figure
out a function's preconditions if they're listed in the function comment
and not buried in the body.

--D

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com