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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx
