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