On Fri, Jul 02, 2021 at 10:17:57AM +0100, Christoph Hellwig wrote:
> 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.
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.
> 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...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
