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