From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org, Dave Chinner <david@fromorbit.com>
Subject: Re: [PATCH 05/14] xfs: remove the aborted parameter to xlog_state_done_syncing
Date: Mon, 16 Mar 2020 13:50:41 -0700 [thread overview]
Message-ID: <20200316205041.GK256767@magnolia> (raw)
In-Reply-To: <20200316144233.900390-6-hch@lst.de>
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
>
next prev parent reply other threads:[~2020-03-16 20:50 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-16 14:42 cleanup log I/O error handling v2 Christoph Hellwig
2020-03-16 14:42 ` [PATCH 01/14] xfs: merge xlog_cil_push into xlog_cil_push_work Christoph Hellwig
2020-03-16 19:40 ` Darrick J. Wong
2020-03-17 0:15 ` Dave Chinner
2020-03-17 13:23 ` Brian Foster
2020-03-16 14:42 ` [PATCH 02/14] xfs: factor out a xlog_wait_on_iclog helper Christoph Hellwig
2020-03-16 20:20 ` Darrick J. Wong
2020-03-17 13:23 ` Brian Foster
2020-03-16 14:42 ` [PATCH 03/14] xfs: simplify the xfs_log_release_iclog calling convention Christoph Hellwig
2020-03-16 20:21 ` Darrick J. Wong
2020-03-17 13:23 ` Brian Foster
2020-03-16 14:42 ` [PATCH 04/14] xfs: simplify log shutdown checking in xfs_log_release_iclog Christoph Hellwig
2020-03-16 20:33 ` Darrick J. Wong
2020-03-17 13:24 ` Brian Foster
2020-03-16 14:42 ` [PATCH 05/14] xfs: remove the aborted parameter to xlog_state_done_syncing Christoph Hellwig
2020-03-16 20:50 ` Darrick J. Wong [this message]
2020-03-18 9:38 ` Christoph Hellwig
2020-03-17 13:24 ` Brian Foster
2020-03-16 14:42 ` [PATCH 06/14] xfs: refactor xlog_state_clean_iclog Christoph Hellwig
2020-03-16 20:59 ` Darrick J. Wong
2020-03-17 13:25 ` Brian Foster
2020-03-18 9:40 ` Christoph Hellwig
2020-03-16 14:42 ` [PATCH 07/14] xfs: move the ioerror check out of xlog_state_clean_iclog Christoph Hellwig
2020-03-16 21:00 ` Darrick J. Wong
2020-03-17 13:25 ` Brian Foster
2020-03-16 14:42 ` [PATCH 08/14] xfs: move xlog_state_do_iclog_callbacks up Christoph Hellwig
2020-03-16 21:00 ` Darrick J. Wong
2020-03-18 14:44 ` Brian Foster
2020-03-16 14:42 ` [PATCH 09/14] xfs: move log shut down handling out of xlog_state_iodone_process_iclog Christoph Hellwig
2020-03-16 21:02 ` Darrick J. Wong
2020-03-18 14:48 ` Brian Foster
2020-03-18 16:34 ` Christoph Hellwig
2020-03-19 11:36 ` Brian Foster
2020-03-19 13:05 ` Christoph Hellwig
2020-03-19 13:37 ` Brian Foster
2020-03-16 14:42 ` [PATCH 10/14] xfs: refactor xlog_state_iodone_process_iclog Christoph Hellwig
2020-03-16 21:07 ` Darrick J. Wong
2020-03-16 14:42 ` [PATCH 11/14] xfs: merge xlog_state_clean_iclog into xlog_state_iodone_process_iclog Christoph Hellwig
2020-03-16 21:09 ` Darrick J. Wong
2020-03-18 14:48 ` Brian Foster
2020-03-16 14:42 ` [PATCH 12/14] xfs: merge xlog_state_set_callback " Christoph Hellwig
2020-03-16 21:10 ` Darrick J. Wong
2020-03-18 14:48 ` Brian Foster
2020-03-16 14:42 ` [PATCH 13/14] xfs: remove xlog_state_want_sync Christoph Hellwig
2020-03-16 21:23 ` Darrick J. Wong
2020-03-18 14:48 ` Brian Foster
2020-03-16 14:42 ` [PATCH 14/14] xfs: remove XLOG_STATE_IOERROR Christoph Hellwig
2020-03-16 21:25 ` Darrick J. Wong
2020-03-18 9:43 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200316205041.GK256767@magnolia \
--to=darrick.wong@oracle.com \
--cc=david@fromorbit.com \
--cc=hch@lst.de \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox