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