| Message ID | 20210617082617.971602-8-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: log fixes for for-next | expand |
On Thu, Jun 17, 2021 at 06:26:16PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > We currently attach iclog callbacks for the CIL when the commit > iclog is returned from xlog_write. Because > xlog_state_get_iclog_space() always guarantees that the commit > record will fit in the iclog it returns, we can move this IO > callback setting to xlog_cil_set_ctx_write_state(), record the > commit iclog in the context and remove the need for the commit iclog > to be returned by xlog_write() altogether. > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_log.c | 8 ++---- > fs/xfs/xfs_log_cil.c | 65 +++++++++++++++++++++++++------------------ > fs/xfs/xfs_log_priv.h | 3 +- > 3 files changed, 42 insertions(+), 34 deletions(-) > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index 1c214b395223..359246d54db7 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, NULL, &lv_chain, ticket, NULL, reg.i_len); > + return xlog_write(log, NULL, &lv_chain, ticket, reg.i_len); > } > > /* > @@ -2386,7 +2386,6 @@ xlog_write( > struct xfs_cil_ctx *ctx, > struct list_head *lv_chain, > struct xlog_ticket *ticket, > - struct xlog_in_core **commit_iclog, > uint32_t len) > { > struct xlog_in_core *iclog = NULL; > @@ -2436,10 +2435,7 @@ xlog_write( > */ > spin_lock(&log->l_icloglock); > xlog_state_finish_copy(log, iclog, record_cnt, 0); > - if (commit_iclog) > - *commit_iclog = iclog; > - else > - error = xlog_state_release_iclog(log, iclog, ticket); > + error = xlog_state_release_iclog(log, iclog, ticket); > spin_unlock(&log->l_icloglock); > > return error; > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c > index 2d8d904ffb78..87e30917ce2e 100644 > --- a/fs/xfs/xfs_log_cil.c > +++ b/fs/xfs/xfs_log_cil.c > @@ -799,11 +799,34 @@ xlog_cil_set_ctx_write_state( > > ASSERT(!ctx->commit_lsn); > spin_lock(&cil->xc_push_lock); > - if (!ctx->start_lsn) > + if (!ctx->start_lsn) { > ctx->start_lsn = lsn; > - else > - ctx->commit_lsn = lsn; > + spin_unlock(&cil->xc_push_lock); > + return; > + } > + > + /* > + * Take a reference to the iclog for the context so that we still hold > + * it when xlog_write is done and has released it. This means the > + * context controls when the iclog is released for IO. > + */ > + atomic_inc(&iclog->ic_refcnt); Where do we drop this refcount? Is this the accounting adjustment that we have to make because xlog_write always decrements the iclog refcount now? > + ctx->commit_iclog = iclog; > + ctx->commit_lsn = lsn; > spin_unlock(&cil->xc_push_lock); I've noticed how the setting of ctx->commit_lsn has moved to before the point where we splice callback lists, only to move them back below in the next patch. That has made it harder for me to understand this series. I /think/ the goal of this patch is not really a functional change so much as a refactoring to make the cil context track the commit iclog directly and then smooth out some of the refcounting code, but the shuffling around of these variables makes me wonder if I'm missing some other subtlety. --D > + > + /* > + * xlog_state_get_iclog_space() guarantees there is enough space in the > + * iclog for an entire commit record, so attach the context callbacks to > + * the iclog at this time if we are not already in a shutdown state. > + */ > + spin_lock(&iclog->ic_callback_lock); > + if (iclog->ic_state == XLOG_STATE_IOERROR) { > + spin_unlock(&iclog->ic_callback_lock); > + return; > + } > + list_add_tail(&ctx->iclog_entry, &iclog->ic_callbacks); > + spin_unlock(&iclog->ic_callback_lock); > } > > /* > @@ -858,8 +881,7 @@ xlog_cil_order_write( > */ > int > xlog_cil_write_commit_record( > - struct xfs_cil_ctx *ctx, > - struct xlog_in_core **iclog) > + struct xfs_cil_ctx *ctx) > { > struct xlog *log = ctx->cil->xc_log; > struct xlog_op_header ophdr = { > @@ -890,7 +912,7 @@ xlog_cil_write_commit_record( > > /* account for space used by record data */ > ctx->ticket->t_curr_res -= reg.i_len; > - error = xlog_write(log, ctx, &lv_chain, ctx->ticket, iclog, reg.i_len); > + error = xlog_write(log, ctx, &lv_chain, ctx->ticket, reg.i_len); > if (error) > xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR); > return error; > @@ -940,7 +962,6 @@ xlog_cil_push_work( > struct xlog *log = cil->xc_log; > struct xfs_log_vec *lv; > struct xfs_cil_ctx *new_ctx; > - struct xlog_in_core *commit_iclog; > int num_iovecs = 0; > int num_bytes = 0; > int error = 0; > @@ -1109,8 +1130,7 @@ xlog_cil_push_work( > * use the commit record lsn then we can move the tail beyond the grant > * write head. > */ > - error = xlog_write(log, ctx, &ctx->lv_chain, ctx->ticket, > - NULL, num_bytes); > + error = xlog_write(log, ctx, &ctx->lv_chain, ctx->ticket, num_bytes); > > /* > * Take the lvhdr back off the lv_chain as it should not be passed > @@ -1120,20 +1140,10 @@ xlog_cil_push_work( > if (error) > goto out_abort_free_ticket; > > - error = xlog_cil_write_commit_record(ctx, &commit_iclog); > + error = xlog_cil_write_commit_record(ctx); > if (error) > goto out_abort_free_ticket; > > - spin_lock(&commit_iclog->ic_callback_lock); > - if (commit_iclog->ic_state == XLOG_STATE_IOERROR) { > - spin_unlock(&commit_iclog->ic_callback_lock); > - goto out_abort_free_ticket; > - } > - ASSERT_ALWAYS(commit_iclog->ic_state == XLOG_STATE_ACTIVE || > - commit_iclog->ic_state == XLOG_STATE_WANT_SYNC); > - list_add_tail(&ctx->iclog_entry, &commit_iclog->ic_callbacks); > - spin_unlock(&commit_iclog->ic_callback_lock); > - > /* > * now the checkpoint commit is complete and we've attached the > * callbacks to the iclog we can assign the commit LSN to the context > @@ -1168,8 +1178,8 @@ xlog_cil_push_work( > if (ctx->start_lsn != commit_lsn) { > struct xlog_in_core *iclog; > > - for (iclog = commit_iclog->ic_prev; > - iclog != commit_iclog; > + for (iclog = ctx->commit_iclog->ic_prev; > + iclog != ctx->commit_iclog; > iclog = iclog->ic_prev) { > xfs_lsn_t hlsn; > > @@ -1201,7 +1211,7 @@ xlog_cil_push_work( > * ordering for this checkpoint is correctly preserved down to > * stable storage. > */ > - commit_iclog->ic_flags |= XLOG_ICL_NEED_FLUSH; > + ctx->commit_iclog->ic_flags |= XLOG_ICL_NEED_FLUSH; > } > > /* > @@ -1214,10 +1224,11 @@ xlog_cil_push_work( > * will be written when released, switch it's state to WANT_SYNC right > * now. > */ > - commit_iclog->ic_flags |= XLOG_ICL_NEED_FUA; > - if (push_commit_stable && commit_iclog->ic_state == XLOG_STATE_ACTIVE) > - xlog_state_switch_iclogs(log, commit_iclog, 0); > - xlog_state_release_iclog(log, commit_iclog, ticket); > + ctx->commit_iclog->ic_flags |= XLOG_ICL_NEED_FUA; > + if (push_commit_stable && > + ctx->commit_iclog->ic_state == XLOG_STATE_ACTIVE) > + xlog_state_switch_iclogs(log, ctx->commit_iclog, 0); > + xlog_state_release_iclog(log, ctx->commit_iclog, ticket); > spin_unlock(&log->l_icloglock); > > xfs_log_ticket_ungrant(log, ticket); > diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h > index 849ba2eb3483..72dfa3b89513 100644 > --- a/fs/xfs/xfs_log_priv.h > +++ b/fs/xfs/xfs_log_priv.h > @@ -237,6 +237,7 @@ struct xfs_cil_ctx { > struct work_struct discard_endio_work; > struct work_struct push_work; > atomic_t order_id; > + struct xlog_in_core *commit_iclog; > }; > > /* > @@ -489,7 +490,7 @@ void xlog_print_tic_res(struct xfs_mount *mp, struct xlog_ticket *ticket); > void xlog_print_trans(struct xfs_trans *); > int xlog_write(struct xlog *log, struct xfs_cil_ctx *ctx, > struct list_head *lv_chain, struct xlog_ticket *tic, > - struct xlog_in_core **commit_iclog, uint32_t len); > + uint32_t len); > > void xfs_log_ticket_ungrant(struct xlog *log, struct xlog_ticket *ticket); > void xfs_log_ticket_regrant(struct xlog *log, struct xlog_ticket *ticket); > -- > 2.31.1 >
On Thu, Jun 17, 2021 at 01:55:52PM -0700, Darrick J. Wong wrote: > On Thu, Jun 17, 2021 at 06:26:16PM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > We currently attach iclog callbacks for the CIL when the commit > > iclog is returned from xlog_write. Because > > xlog_state_get_iclog_space() always guarantees that the commit > > record will fit in the iclog it returns, we can move this IO > > callback setting to xlog_cil_set_ctx_write_state(), record the > > commit iclog in the context and remove the need for the commit iclog > > to be returned by xlog_write() altogether. > > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > --- > > fs/xfs/xfs_log.c | 8 ++---- > > fs/xfs/xfs_log_cil.c | 65 +++++++++++++++++++++++++------------------ > > fs/xfs/xfs_log_priv.h | 3 +- > > 3 files changed, 42 insertions(+), 34 deletions(-) > > > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > > index 1c214b395223..359246d54db7 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, NULL, &lv_chain, ticket, NULL, reg.i_len); > > + return xlog_write(log, NULL, &lv_chain, ticket, reg.i_len); > > } > > > > /* > > @@ -2386,7 +2386,6 @@ xlog_write( > > struct xfs_cil_ctx *ctx, > > struct list_head *lv_chain, > > struct xlog_ticket *ticket, > > - struct xlog_in_core **commit_iclog, > > uint32_t len) > > { > > struct xlog_in_core *iclog = NULL; > > @@ -2436,10 +2435,7 @@ xlog_write( > > */ > > spin_lock(&log->l_icloglock); > > xlog_state_finish_copy(log, iclog, record_cnt, 0); > > - if (commit_iclog) > > - *commit_iclog = iclog; > > - else > > - error = xlog_state_release_iclog(log, iclog, ticket); > > + error = xlog_state_release_iclog(log, iclog, ticket); > > spin_unlock(&log->l_icloglock); > > > > return error; > > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c > > index 2d8d904ffb78..87e30917ce2e 100644 > > --- a/fs/xfs/xfs_log_cil.c > > +++ b/fs/xfs/xfs_log_cil.c > > @@ -799,11 +799,34 @@ xlog_cil_set_ctx_write_state( > > > > ASSERT(!ctx->commit_lsn); > > spin_lock(&cil->xc_push_lock); > > - if (!ctx->start_lsn) > > + if (!ctx->start_lsn) { > > ctx->start_lsn = lsn; > > - else > > - ctx->commit_lsn = lsn; > > + spin_unlock(&cil->xc_push_lock); > > + return; > > + } > > + > > + /* > > + * Take a reference to the iclog for the context so that we still hold > > + * it when xlog_write is done and has released it. This means the > > + * context controls when the iclog is released for IO. > > + */ > > + atomic_inc(&iclog->ic_refcnt); > > Where do we drop this refcount? In xlog_cil_push_work() where we call xlog_state_release_iclog(). > Is this the accounting adjustment that > we have to make because xlog_write always decrements the iclog refcount > now? Yes. > > + ctx->commit_iclog = iclog; > > + ctx->commit_lsn = lsn; > > spin_unlock(&cil->xc_push_lock); > > I've noticed how the setting of ctx->commit_lsn has moved to before the > point where we splice callback lists, only to move them back below in > the next patch. That has made it harder for me to understand this > series. > > I /think/ the goal of this patch is not really a functional change so > much as a refactoring to make the cil context track the commit iclog > directly and then smooth out some of the refcounting code, but the > shuffling around of these variables makes me wonder if I'm missing some > other subtlety. The subtlety is that we can't issue the wakup on the commit_lsn until after the callbacks are attached to the commit iclog. When we set ctx->commit_lsn doesn't really matter - I'm trying to keep the order of "callbacks attached before we issue the wakeup" so that when the waiter is woken and then adds it's callbacks to the same iclog they will be appended to the list after the first commit record's callbacks and hence they get processed in the correct order when journal IO completion runs the callbacks on that iclog. This patch doesn't move the wakeup from after the xlog_write() call completes, so the ordering of setting ctx->commit_lsn and attaching the callbacks inside xlog_write() doesn't really matter. In the next patch, the wakeups move inside xlog_write()->xlog_cil_set_ctx_write_state(), and so now it has to ensure that the ordering is correct. I'll rework the patches so that this one sets up the order the next patch requires rather than minimal change in this patch and reorder in the next patch... Cheers, Dave.
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index 1c214b395223..359246d54db7 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, NULL, &lv_chain, ticket, NULL, reg.i_len); + return xlog_write(log, NULL, &lv_chain, ticket, reg.i_len); } /* @@ -2386,7 +2386,6 @@ xlog_write( struct xfs_cil_ctx *ctx, struct list_head *lv_chain, struct xlog_ticket *ticket, - struct xlog_in_core **commit_iclog, uint32_t len) { struct xlog_in_core *iclog = NULL; @@ -2436,10 +2435,7 @@ xlog_write( */ spin_lock(&log->l_icloglock); xlog_state_finish_copy(log, iclog, record_cnt, 0); - if (commit_iclog) - *commit_iclog = iclog; - else - error = xlog_state_release_iclog(log, iclog, ticket); + error = xlog_state_release_iclog(log, iclog, ticket); spin_unlock(&log->l_icloglock); return error; diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c index 2d8d904ffb78..87e30917ce2e 100644 --- a/fs/xfs/xfs_log_cil.c +++ b/fs/xfs/xfs_log_cil.c @@ -799,11 +799,34 @@ xlog_cil_set_ctx_write_state( ASSERT(!ctx->commit_lsn); spin_lock(&cil->xc_push_lock); - if (!ctx->start_lsn) + if (!ctx->start_lsn) { ctx->start_lsn = lsn; - else - ctx->commit_lsn = lsn; + spin_unlock(&cil->xc_push_lock); + return; + } + + /* + * Take a reference to the iclog for the context so that we still hold + * it when xlog_write is done and has released it. This means the + * context controls when the iclog is released for IO. + */ + atomic_inc(&iclog->ic_refcnt); + ctx->commit_iclog = iclog; + ctx->commit_lsn = lsn; spin_unlock(&cil->xc_push_lock); + + /* + * xlog_state_get_iclog_space() guarantees there is enough space in the + * iclog for an entire commit record, so attach the context callbacks to + * the iclog at this time if we are not already in a shutdown state. + */ + spin_lock(&iclog->ic_callback_lock); + if (iclog->ic_state == XLOG_STATE_IOERROR) { + spin_unlock(&iclog->ic_callback_lock); + return; + } + list_add_tail(&ctx->iclog_entry, &iclog->ic_callbacks); + spin_unlock(&iclog->ic_callback_lock); } /* @@ -858,8 +881,7 @@ xlog_cil_order_write( */ int xlog_cil_write_commit_record( - struct xfs_cil_ctx *ctx, - struct xlog_in_core **iclog) + struct xfs_cil_ctx *ctx) { struct xlog *log = ctx->cil->xc_log; struct xlog_op_header ophdr = { @@ -890,7 +912,7 @@ xlog_cil_write_commit_record( /* account for space used by record data */ ctx->ticket->t_curr_res -= reg.i_len; - error = xlog_write(log, ctx, &lv_chain, ctx->ticket, iclog, reg.i_len); + error = xlog_write(log, ctx, &lv_chain, ctx->ticket, reg.i_len); if (error) xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR); return error; @@ -940,7 +962,6 @@ xlog_cil_push_work( struct xlog *log = cil->xc_log; struct xfs_log_vec *lv; struct xfs_cil_ctx *new_ctx; - struct xlog_in_core *commit_iclog; int num_iovecs = 0; int num_bytes = 0; int error = 0; @@ -1109,8 +1130,7 @@ xlog_cil_push_work( * use the commit record lsn then we can move the tail beyond the grant * write head. */ - error = xlog_write(log, ctx, &ctx->lv_chain, ctx->ticket, - NULL, num_bytes); + error = xlog_write(log, ctx, &ctx->lv_chain, ctx->ticket, num_bytes); /* * Take the lvhdr back off the lv_chain as it should not be passed @@ -1120,20 +1140,10 @@ xlog_cil_push_work( if (error) goto out_abort_free_ticket; - error = xlog_cil_write_commit_record(ctx, &commit_iclog); + error = xlog_cil_write_commit_record(ctx); if (error) goto out_abort_free_ticket; - spin_lock(&commit_iclog->ic_callback_lock); - if (commit_iclog->ic_state == XLOG_STATE_IOERROR) { - spin_unlock(&commit_iclog->ic_callback_lock); - goto out_abort_free_ticket; - } - ASSERT_ALWAYS(commit_iclog->ic_state == XLOG_STATE_ACTIVE || - commit_iclog->ic_state == XLOG_STATE_WANT_SYNC); - list_add_tail(&ctx->iclog_entry, &commit_iclog->ic_callbacks); - spin_unlock(&commit_iclog->ic_callback_lock); - /* * now the checkpoint commit is complete and we've attached the * callbacks to the iclog we can assign the commit LSN to the context @@ -1168,8 +1178,8 @@ xlog_cil_push_work( if (ctx->start_lsn != commit_lsn) { struct xlog_in_core *iclog; - for (iclog = commit_iclog->ic_prev; - iclog != commit_iclog; + for (iclog = ctx->commit_iclog->ic_prev; + iclog != ctx->commit_iclog; iclog = iclog->ic_prev) { xfs_lsn_t hlsn; @@ -1201,7 +1211,7 @@ xlog_cil_push_work( * ordering for this checkpoint is correctly preserved down to * stable storage. */ - commit_iclog->ic_flags |= XLOG_ICL_NEED_FLUSH; + ctx->commit_iclog->ic_flags |= XLOG_ICL_NEED_FLUSH; } /* @@ -1214,10 +1224,11 @@ xlog_cil_push_work( * will be written when released, switch it's state to WANT_SYNC right * now. */ - commit_iclog->ic_flags |= XLOG_ICL_NEED_FUA; - if (push_commit_stable && commit_iclog->ic_state == XLOG_STATE_ACTIVE) - xlog_state_switch_iclogs(log, commit_iclog, 0); - xlog_state_release_iclog(log, commit_iclog, ticket); + ctx->commit_iclog->ic_flags |= XLOG_ICL_NEED_FUA; + if (push_commit_stable && + ctx->commit_iclog->ic_state == XLOG_STATE_ACTIVE) + xlog_state_switch_iclogs(log, ctx->commit_iclog, 0); + xlog_state_release_iclog(log, ctx->commit_iclog, ticket); spin_unlock(&log->l_icloglock); xfs_log_ticket_ungrant(log, ticket); diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h index 849ba2eb3483..72dfa3b89513 100644 --- a/fs/xfs/xfs_log_priv.h +++ b/fs/xfs/xfs_log_priv.h @@ -237,6 +237,7 @@ struct xfs_cil_ctx { struct work_struct discard_endio_work; struct work_struct push_work; atomic_t order_id; + struct xlog_in_core *commit_iclog; }; /* @@ -489,7 +490,7 @@ void xlog_print_tic_res(struct xfs_mount *mp, struct xlog_ticket *ticket); void xlog_print_trans(struct xfs_trans *); int xlog_write(struct xlog *log, struct xfs_cil_ctx *ctx, struct list_head *lv_chain, struct xlog_ticket *tic, - struct xlog_in_core **commit_iclog, uint32_t len); + uint32_t len); void xfs_log_ticket_ungrant(struct xlog *log, struct xlog_ticket *ticket); void xfs_log_ticket_regrant(struct xlog *log, struct xlog_ticket *ticket);