| Message ID | 20200316144233.900390-6-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [01/14] xfs: merge xlog_cil_push into xlog_cil_push_work | expand |
On Mon, Mar 16, 2020 at 03:42:24PM +0100, Christoph Hellwig wrote: > We can just check for a shut down log all the way down in > xlog_cil_committed instead of passing the parameter. This means a > slight behavior change in that we now also abort log items if the > shutdown came in halfway into the I/O completion processing, which > actually is the right thing to do. "if the shutdown came in halfway into the I/O completion..." Does this refer to a shutdown triggered by some other thread? As in: this thread is processing log IO completion; meanwhile someone on the front end cancels a dirty transaction and causes the fs/log to shut down; and so this thread now tells all the log items that they were aborted? Whereas before, the only reason we'd tell the log items that they were aborted is if the IO completion itself signalled some kind of error? --D > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_log.c | 48 +++++++++++++++----------------------------- > fs/xfs/xfs_log.h | 2 +- > fs/xfs/xfs_log_cil.c | 12 +++++------ > 3 files changed, 23 insertions(+), 39 deletions(-) > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index 7af9c292540b..8ede2852f104 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -47,8 +47,7 @@ xlog_dealloc_log( > > /* local state machine functions */ > STATIC void xlog_state_done_syncing( > - struct xlog_in_core *iclog, > - bool aborted); > + struct xlog_in_core *iclog); > STATIC int > xlog_state_get_iclog_space( > struct xlog *log, > @@ -1254,7 +1253,6 @@ xlog_ioend_work( > struct xlog_in_core *iclog = > container_of(work, struct xlog_in_core, ic_end_io_work); > struct xlog *log = iclog->ic_log; > - bool aborted = false; > int error; > > error = blk_status_to_errno(iclog->ic_bio.bi_status); > @@ -1270,17 +1268,9 @@ xlog_ioend_work( > if (XFS_TEST_ERROR(error, log->l_mp, XFS_ERRTAG_IODONE_IOERR)) { > xfs_alert(log->l_mp, "log I/O error %d", error); > xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR); > - /* > - * This flag will be propagated to the trans-committed > - * callback routines to let them know that the log-commit > - * didn't succeed. > - */ > - aborted = true; > - } else if (iclog->ic_state == XLOG_STATE_IOERROR) { > - aborted = true; > } > > - xlog_state_done_syncing(iclog, aborted); > + xlog_state_done_syncing(iclog); > bio_uninit(&iclog->ic_bio); > > /* > @@ -1759,7 +1749,7 @@ xlog_write_iclog( > * the buffer manually, the code needs to be kept in sync > * with the I/O completion path. > */ > - xlog_state_done_syncing(iclog, true); > + xlog_state_done_syncing(iclog); > up(&iclog->ic_sema); > return; > } > @@ -2783,8 +2773,7 @@ xlog_state_iodone_process_iclog( > static void > xlog_state_do_iclog_callbacks( > struct xlog *log, > - struct xlog_in_core *iclog, > - bool aborted) > + struct xlog_in_core *iclog) > __releases(&log->l_icloglock) > __acquires(&log->l_icloglock) > { > @@ -2796,7 +2785,7 @@ xlog_state_do_iclog_callbacks( > list_splice_init(&iclog->ic_callbacks, &tmp); > > spin_unlock(&iclog->ic_callback_lock); > - xlog_cil_process_committed(&tmp, aborted); > + xlog_cil_process_committed(&tmp); > spin_lock(&iclog->ic_callback_lock); > } > > @@ -2811,8 +2800,7 @@ xlog_state_do_iclog_callbacks( > > STATIC void > xlog_state_do_callback( > - struct xlog *log, > - bool aborted) > + struct xlog *log) > { > struct xlog_in_core *iclog; > struct xlog_in_core *first_iclog; > @@ -2853,7 +2841,7 @@ xlog_state_do_callback( > * we'll have to run at least one more complete loop. > */ > cycled_icloglock = true; > - xlog_state_do_iclog_callbacks(log, iclog, aborted); > + xlog_state_do_iclog_callbacks(log, iclog); > > xlog_state_clean_iclog(log, iclog); > iclog = iclog->ic_next; > @@ -2891,25 +2879,22 @@ xlog_state_do_callback( > */ > STATIC void > xlog_state_done_syncing( > - struct xlog_in_core *iclog, > - bool aborted) > + struct xlog_in_core *iclog) > { > struct xlog *log = iclog->ic_log; > > spin_lock(&log->l_icloglock); > - > ASSERT(atomic_read(&iclog->ic_refcnt) == 0); > > /* > * If we got an error, either on the first buffer, or in the case of > - * split log writes, on the second, we mark ALL iclogs STATE_IOERROR, > - * and none should ever be attempted to be written to disk > - * again. > + * split log writes, on the second, we shut down the file system and > + * none should ever be attempted to be written to disk again. > */ > - if (iclog->ic_state == XLOG_STATE_SYNCING) > + if (!XLOG_FORCED_SHUTDOWN(log)) { > + ASSERT(iclog->ic_state == XLOG_STATE_SYNCING); > iclog->ic_state = XLOG_STATE_DONE_SYNC; > - else > - ASSERT(iclog->ic_state == XLOG_STATE_IOERROR); > + } > > /* > * Someone could be sleeping prior to writing out the next > @@ -2918,9 +2903,8 @@ xlog_state_done_syncing( > */ > wake_up_all(&iclog->ic_write_wait); > spin_unlock(&log->l_icloglock); > - xlog_state_do_callback(log, aborted); /* also cleans log */ > -} /* xlog_state_done_syncing */ > - > + xlog_state_do_callback(log); /* also cleans log */ > +} > > /* > * If the head of the in-core log ring is not (ACTIVE or DIRTY), then we must > @@ -3884,7 +3868,7 @@ xfs_log_force_umount( > spin_lock(&log->l_cilp->xc_push_lock); > wake_up_all(&log->l_cilp->xc_commit_wait); > spin_unlock(&log->l_cilp->xc_push_lock); > - xlog_state_do_callback(log, true); > + xlog_state_do_callback(log); > > /* return non-zero if log IOERROR transition had already happened */ > return retval; > diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h > index b38602216c5a..cc77cc36560a 100644 > --- a/fs/xfs/xfs_log.h > +++ b/fs/xfs/xfs_log.h > @@ -137,7 +137,7 @@ void xfs_log_ticket_put(struct xlog_ticket *ticket); > > void xfs_log_commit_cil(struct xfs_mount *mp, struct xfs_trans *tp, > xfs_lsn_t *commit_lsn, bool regrant); > -void xlog_cil_process_committed(struct list_head *list, bool aborted); > +void xlog_cil_process_committed(struct list_head *list); > bool xfs_log_item_in_current_chkpt(struct xfs_log_item *lip); > > void xfs_log_work_queue(struct xfs_mount *mp); > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c > index 047ac253edfe..adc4af336c97 100644 > --- a/fs/xfs/xfs_log_cil.c > +++ b/fs/xfs/xfs_log_cil.c > @@ -574,10 +574,10 @@ xlog_discard_busy_extents( > */ > static void > xlog_cil_committed( > - struct xfs_cil_ctx *ctx, > - bool abort) > + struct xfs_cil_ctx *ctx) > { > struct xfs_mount *mp = ctx->cil->xc_log->l_mp; > + bool abort = XLOG_FORCED_SHUTDOWN(ctx->cil->xc_log); > > /* > * If the I/O failed, we're aborting the commit and already shutdown. > @@ -613,15 +613,14 @@ xlog_cil_committed( > > void > xlog_cil_process_committed( > - struct list_head *list, > - bool aborted) > + struct list_head *list) > { > struct xfs_cil_ctx *ctx; > > while ((ctx = list_first_entry_or_null(list, > struct xfs_cil_ctx, iclog_entry))) { > list_del(&ctx->iclog_entry); > - xlog_cil_committed(ctx, aborted); > + xlog_cil_committed(ctx); > } > } > > @@ -878,7 +877,8 @@ xlog_cil_push_work( > out_abort_free_ticket: > xfs_log_ticket_put(tic); > out_abort: > - xlog_cil_committed(ctx, true); > + ASSERT(XLOG_FORCED_SHUTDOWN(log)); > + xlog_cil_committed(ctx); > } > > /* > -- > 2.24.1 >
On Mon, Mar 16, 2020 at 03:42:24PM +0100, Christoph Hellwig wrote: > We can just check for a shut down log all the way down in > xlog_cil_committed instead of passing the parameter. This means a > slight behavior change in that we now also abort log items if the > shutdown came in halfway into the I/O completion processing, which > actually is the right thing to do. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_log.c | 48 +++++++++++++++----------------------------- > fs/xfs/xfs_log.h | 2 +- > fs/xfs/xfs_log_cil.c | 12 +++++------ > 3 files changed, 23 insertions(+), 39 deletions(-) > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index 7af9c292540b..8ede2852f104 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c ... > @@ -2891,25 +2879,22 @@ xlog_state_do_callback( > */ > STATIC void > xlog_state_done_syncing( > - struct xlog_in_core *iclog, > - bool aborted) > + struct xlog_in_core *iclog) > { > struct xlog *log = iclog->ic_log; > > spin_lock(&log->l_icloglock); > - > ASSERT(atomic_read(&iclog->ic_refcnt) == 0); > > /* > * If we got an error, either on the first buffer, or in the case of > - * split log writes, on the second, we mark ALL iclogs STATE_IOERROR, > - * and none should ever be attempted to be written to disk > - * again. > + * split log writes, on the second, we shut down the file system and > + * none should ever be attempted to be written to disk again. "none" refers to iclogs in the original comment, which has been removed. This could probably read "... and iclogs should never ..." instead. With that fixed: Reviewed-by: Brian Foster <bfoster@redhat.com> > */ > - if (iclog->ic_state == XLOG_STATE_SYNCING) > + if (!XLOG_FORCED_SHUTDOWN(log)) { > + ASSERT(iclog->ic_state == XLOG_STATE_SYNCING); > iclog->ic_state = XLOG_STATE_DONE_SYNC; > - else > - ASSERT(iclog->ic_state == XLOG_STATE_IOERROR); > + } > > /* > * Someone could be sleeping prior to writing out the next > @@ -2918,9 +2903,8 @@ xlog_state_done_syncing( > */ > wake_up_all(&iclog->ic_write_wait); > spin_unlock(&log->l_icloglock); > - xlog_state_do_callback(log, aborted); /* also cleans log */ > -} /* xlog_state_done_syncing */ > - > + xlog_state_do_callback(log); /* also cleans log */ > +} > > /* > * If the head of the in-core log ring is not (ACTIVE or DIRTY), then we must > @@ -3884,7 +3868,7 @@ xfs_log_force_umount( > spin_lock(&log->l_cilp->xc_push_lock); > wake_up_all(&log->l_cilp->xc_commit_wait); > spin_unlock(&log->l_cilp->xc_push_lock); > - xlog_state_do_callback(log, true); > + xlog_state_do_callback(log); > > /* return non-zero if log IOERROR transition had already happened */ > return retval; > diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h > index b38602216c5a..cc77cc36560a 100644 > --- a/fs/xfs/xfs_log.h > +++ b/fs/xfs/xfs_log.h > @@ -137,7 +137,7 @@ void xfs_log_ticket_put(struct xlog_ticket *ticket); > > void xfs_log_commit_cil(struct xfs_mount *mp, struct xfs_trans *tp, > xfs_lsn_t *commit_lsn, bool regrant); > -void xlog_cil_process_committed(struct list_head *list, bool aborted); > +void xlog_cil_process_committed(struct list_head *list); > bool xfs_log_item_in_current_chkpt(struct xfs_log_item *lip); > > void xfs_log_work_queue(struct xfs_mount *mp); > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c > index 047ac253edfe..adc4af336c97 100644 > --- a/fs/xfs/xfs_log_cil.c > +++ b/fs/xfs/xfs_log_cil.c > @@ -574,10 +574,10 @@ xlog_discard_busy_extents( > */ > static void > xlog_cil_committed( > - struct xfs_cil_ctx *ctx, > - bool abort) > + struct xfs_cil_ctx *ctx) > { > struct xfs_mount *mp = ctx->cil->xc_log->l_mp; > + bool abort = XLOG_FORCED_SHUTDOWN(ctx->cil->xc_log); > > /* > * If the I/O failed, we're aborting the commit and already shutdown. > @@ -613,15 +613,14 @@ xlog_cil_committed( > > void > xlog_cil_process_committed( > - struct list_head *list, > - bool aborted) > + struct list_head *list) > { > struct xfs_cil_ctx *ctx; > > while ((ctx = list_first_entry_or_null(list, > struct xfs_cil_ctx, iclog_entry))) { > list_del(&ctx->iclog_entry); > - xlog_cil_committed(ctx, aborted); > + xlog_cil_committed(ctx); > } > } > > @@ -878,7 +877,8 @@ xlog_cil_push_work( > out_abort_free_ticket: > xfs_log_ticket_put(tic); > out_abort: > - xlog_cil_committed(ctx, true); > + ASSERT(XLOG_FORCED_SHUTDOWN(log)); > + xlog_cil_committed(ctx); > } > > /* > -- > 2.24.1 >
On Mon, Mar 16, 2020 at 01:50:41PM -0700, Darrick J. Wong wrote: > On Mon, Mar 16, 2020 at 03:42:24PM +0100, Christoph Hellwig wrote: > > We can just check for a shut down log all the way down in > > xlog_cil_committed instead of passing the parameter. This means a > > slight behavior change in that we now also abort log items if the > > shutdown came in halfway into the I/O completion processing, which > > actually is the right thing to do. > > "if the shutdown came in halfway into the I/O completion..." > > Does this refer to a shutdown triggered by some other thread? As in: > this thread is processing log IO completion; meanwhile someone on the > front end cancels a dirty transaction and causes the fs/log to shut > down; and so this thread now tells all the log items that they were > aborted? Whereas before, the only reason we'd tell the log items that > they were aborted is if the IO completion itself signalled some kind of > error? No, before we also checked for XLOG_STATE_IOERROR in ẋlog_ioend_work. But the I/O end processing drops l_icloglock in various places, in which case another thread could shut down the log as well.
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index 7af9c292540b..8ede2852f104 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -47,8 +47,7 @@ xlog_dealloc_log( /* local state machine functions */ STATIC void xlog_state_done_syncing( - struct xlog_in_core *iclog, - bool aborted); + struct xlog_in_core *iclog); STATIC int xlog_state_get_iclog_space( struct xlog *log, @@ -1254,7 +1253,6 @@ xlog_ioend_work( struct xlog_in_core *iclog = container_of(work, struct xlog_in_core, ic_end_io_work); struct xlog *log = iclog->ic_log; - bool aborted = false; int error; error = blk_status_to_errno(iclog->ic_bio.bi_status); @@ -1270,17 +1268,9 @@ xlog_ioend_work( if (XFS_TEST_ERROR(error, log->l_mp, XFS_ERRTAG_IODONE_IOERR)) { xfs_alert(log->l_mp, "log I/O error %d", error); xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR); - /* - * This flag will be propagated to the trans-committed - * callback routines to let them know that the log-commit - * didn't succeed. - */ - aborted = true; - } else if (iclog->ic_state == XLOG_STATE_IOERROR) { - aborted = true; } - xlog_state_done_syncing(iclog, aborted); + xlog_state_done_syncing(iclog); bio_uninit(&iclog->ic_bio); /* @@ -1759,7 +1749,7 @@ xlog_write_iclog( * the buffer manually, the code needs to be kept in sync * with the I/O completion path. */ - xlog_state_done_syncing(iclog, true); + xlog_state_done_syncing(iclog); up(&iclog->ic_sema); return; } @@ -2783,8 +2773,7 @@ xlog_state_iodone_process_iclog( static void xlog_state_do_iclog_callbacks( struct xlog *log, - struct xlog_in_core *iclog, - bool aborted) + struct xlog_in_core *iclog) __releases(&log->l_icloglock) __acquires(&log->l_icloglock) { @@ -2796,7 +2785,7 @@ xlog_state_do_iclog_callbacks( list_splice_init(&iclog->ic_callbacks, &tmp); spin_unlock(&iclog->ic_callback_lock); - xlog_cil_process_committed(&tmp, aborted); + xlog_cil_process_committed(&tmp); spin_lock(&iclog->ic_callback_lock); } @@ -2811,8 +2800,7 @@ xlog_state_do_iclog_callbacks( STATIC void xlog_state_do_callback( - struct xlog *log, - bool aborted) + struct xlog *log) { struct xlog_in_core *iclog; struct xlog_in_core *first_iclog; @@ -2853,7 +2841,7 @@ xlog_state_do_callback( * we'll have to run at least one more complete loop. */ cycled_icloglock = true; - xlog_state_do_iclog_callbacks(log, iclog, aborted); + xlog_state_do_iclog_callbacks(log, iclog); xlog_state_clean_iclog(log, iclog); iclog = iclog->ic_next; @@ -2891,25 +2879,22 @@ xlog_state_do_callback( */ STATIC void xlog_state_done_syncing( - struct xlog_in_core *iclog, - bool aborted) + struct xlog_in_core *iclog) { struct xlog *log = iclog->ic_log; spin_lock(&log->l_icloglock); - ASSERT(atomic_read(&iclog->ic_refcnt) == 0); /* * If we got an error, either on the first buffer, or in the case of - * split log writes, on the second, we mark ALL iclogs STATE_IOERROR, - * and none should ever be attempted to be written to disk - * again. + * split log writes, on the second, we shut down the file system and + * none should ever be attempted to be written to disk again. */ - if (iclog->ic_state == XLOG_STATE_SYNCING) + if (!XLOG_FORCED_SHUTDOWN(log)) { + ASSERT(iclog->ic_state == XLOG_STATE_SYNCING); iclog->ic_state = XLOG_STATE_DONE_SYNC; - else - ASSERT(iclog->ic_state == XLOG_STATE_IOERROR); + } /* * Someone could be sleeping prior to writing out the next @@ -2918,9 +2903,8 @@ xlog_state_done_syncing( */ wake_up_all(&iclog->ic_write_wait); spin_unlock(&log->l_icloglock); - xlog_state_do_callback(log, aborted); /* also cleans log */ -} /* xlog_state_done_syncing */ - + xlog_state_do_callback(log); /* also cleans log */ +} /* * If the head of the in-core log ring is not (ACTIVE or DIRTY), then we must @@ -3884,7 +3868,7 @@ xfs_log_force_umount( spin_lock(&log->l_cilp->xc_push_lock); wake_up_all(&log->l_cilp->xc_commit_wait); spin_unlock(&log->l_cilp->xc_push_lock); - xlog_state_do_callback(log, true); + xlog_state_do_callback(log); /* return non-zero if log IOERROR transition had already happened */ return retval; diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h index b38602216c5a..cc77cc36560a 100644 --- a/fs/xfs/xfs_log.h +++ b/fs/xfs/xfs_log.h @@ -137,7 +137,7 @@ void xfs_log_ticket_put(struct xlog_ticket *ticket); void xfs_log_commit_cil(struct xfs_mount *mp, struct xfs_trans *tp, xfs_lsn_t *commit_lsn, bool regrant); -void xlog_cil_process_committed(struct list_head *list, bool aborted); +void xlog_cil_process_committed(struct list_head *list); bool xfs_log_item_in_current_chkpt(struct xfs_log_item *lip); void xfs_log_work_queue(struct xfs_mount *mp); diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c index 047ac253edfe..adc4af336c97 100644 --- a/fs/xfs/xfs_log_cil.c +++ b/fs/xfs/xfs_log_cil.c @@ -574,10 +574,10 @@ xlog_discard_busy_extents( */ static void xlog_cil_committed( - struct xfs_cil_ctx *ctx, - bool abort) + struct xfs_cil_ctx *ctx) { struct xfs_mount *mp = ctx->cil->xc_log->l_mp; + bool abort = XLOG_FORCED_SHUTDOWN(ctx->cil->xc_log); /* * If the I/O failed, we're aborting the commit and already shutdown. @@ -613,15 +613,14 @@ xlog_cil_committed( void xlog_cil_process_committed( - struct list_head *list, - bool aborted) + struct list_head *list) { struct xfs_cil_ctx *ctx; while ((ctx = list_first_entry_or_null(list, struct xfs_cil_ctx, iclog_entry))) { list_del(&ctx->iclog_entry); - xlog_cil_committed(ctx, aborted); + xlog_cil_committed(ctx); } } @@ -878,7 +877,8 @@ xlog_cil_push_work( out_abort_free_ticket: xfs_log_ticket_put(tic); out_abort: - xlog_cil_committed(ctx, true); + ASSERT(XLOG_FORCED_SHUTDOWN(log)); + xlog_cil_committed(ctx); } /*
We can just check for a shut down log all the way down in xlog_cil_committed instead of passing the parameter. This means a slight behavior change in that we now also abort log items if the shutdown came in halfway into the I/O completion processing, which actually is the right thing to do. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_log.c | 48 +++++++++++++++----------------------------- fs/xfs/xfs_log.h | 2 +- fs/xfs/xfs_log_cil.c | 12 +++++------ 3 files changed, 23 insertions(+), 39 deletions(-)