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

["Darrick J. Wong" <djwong@kernel.org>]

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

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