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

[Dave Chinner <david@fromorbit.com>]

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.

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com