Re: [PATCH 4/8] xfs: pass a CIL context to xlog_write()

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

On Fri, Jun 18, 2021 at 08:03:37AM +1000, Dave Chinner wrote:
> On Thu, Jun 17, 2021 at 01:24:02PM -0700, Darrick J. Wong wrote:
> > On Thu, Jun 17, 2021 at 06:26:13PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Pass the CIL context to xlog_write() rather than a pointer to a LSN
> > > variable. Only the CIL checkpoint calls to xlog_write() need to know
> > > about the start LSN of the writes, so rework xlog_write to directly
> > > write the LSNs into the CIL context structure.
> > > 
> > > This removes the commit_lsn variable from xlog_cil_push_work(), so
> > > now we only have to issue the commit record ordering wakeup from
> > > there.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > >  fs/xfs/xfs_log.c      | 22 +++++++++++++++++-----
> > >  fs/xfs/xfs_log_cil.c  | 19 ++++++++-----------
> > >  fs/xfs/xfs_log_priv.h |  4 ++--
> > >  3 files changed, 27 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > > index cf661c155786..fc0e43c57683 100644
> > > --- a/fs/xfs/xfs_log.c
> > > +++ b/fs/xfs/xfs_log.c
> > > @@ -871,7 +871,7 @@ xlog_write_unmount_record(
> > >  	 */
> > >  	if (log->l_targ != log->l_mp->m_ddev_targp)
> > >  		blkdev_issue_flush(log->l_targ->bt_bdev);
> > > -	return xlog_write(log, &lv_chain, ticket, NULL, NULL, reg.i_len);
> > > +	return xlog_write(log, NULL, &lv_chain, ticket, NULL, reg.i_len);
> > >  }
> > >  
> > >  /*
> > > @@ -2383,9 +2383,9 @@ xlog_write_partial(
> > >  int
> > >  xlog_write(
> > >  	struct xlog		*log,
> > > +	struct xfs_cil_ctx	*ctx,
> > >  	struct list_head	*lv_chain,
> > >  	struct xlog_ticket	*ticket,
> > > -	xfs_lsn_t		*start_lsn,
> > >  	struct xlog_in_core	**commit_iclog,
> > >  	uint32_t		len)
> > >  {
> > > @@ -2408,9 +2408,21 @@ xlog_write(
> > >  	if (error)
> > >  		return error;
> > >  
> > > -	/* start_lsn is the LSN of the first iclog written to. */
> > > -	if (start_lsn)
> > > -		*start_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> > > +	/*
> > > +	 * If we have a CIL context, record the LSN of the iclog we were just
> > > +	 * granted space to start writing into. If the context doesn't have
> > > +	 * a start_lsn recorded, then this iclog will contain the start record
> > > +	 * for the checkpoint. Otherwise this write contains the commit record
> > > +	 * for the checkpoint.
> > > +	 */
> > > +	if (ctx) {
> > > +		spin_lock(&ctx->cil->xc_push_lock);
> > > +		if (!ctx->start_lsn)
> > > +			ctx->start_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> > > +		else
> > > +			ctx->commit_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
> > > +		spin_unlock(&ctx->cil->xc_push_lock);
> > 
> > This cycling of the push lock when setting start_lsn is new.  What are
> > we protecting against here by taking the lock?
> 
> Later in the series it will be the ordering wakeup when we set the
> start_lsn. The ordering ends with both start_lsn and commit_lsn
> being treated the same way w.r.t. wakeups, so I just started it off
> the same way here.

Ah, right, I see that now that I've gotten to patch 8.

> > Also, just to check my assumptions: why do we take the push lock when
> > setting commit_lsn?  Is that to synchronize with the xc_committing loop
> > that looks for contexts that need pushing?
> 
> Yes - the spinlock provides the memory barriers for access to the
> variable. I could use WRITE_ONCE/READ_ONCE here for this specific patch,
> but the lock is necessary for compound operations in upcoming
> patches so it didn't make any sense to use _ONCE macros here only to
> remove them again later.

Nah, I'd leave it, especially since it's already a little strange that
the place where we set ctx->commit_lsn bounces around relative to the
callback list splicing...

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com