Re: [PATCH 4/5] xfs: attached iclog callbacks in xlog_cil_set_ctx_write_state()

[Christoph Hellwig <hch@infradead.org>]

On Wed, Jun 30, 2021 at 05:21:07PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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/

> --- 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.

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.