From: Brian Foster <bfoster@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org, Dave Chinner <david@fromorbit.com>
Subject: Re: [PATCH 6/8] xfs: refactor xlog_state_clean_iclog
Date: Fri, 20 Mar 2020 07:51:51 -0400 [thread overview]
Message-ID: <20200320115151.GA42814@bfoster> (raw)
In-Reply-To: <20200320065311.28134-7-hch@lst.de>
On Fri, Mar 20, 2020 at 07:53:09AM +0100, Christoph Hellwig wrote:
> Factor out a few self-contained helpers from xlog_state_clean_iclog, and
> update the documentation so it primarily documents why things happens
> instead of how.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_log.c | 180 +++++++++++++++++++++++------------------------
> 1 file changed, 88 insertions(+), 92 deletions(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 4f9303524efb..6facbb91b8a8 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2540,111 +2540,107 @@ xlog_write(
> *****************************************************************************
> */
>
> +static void
> +xlog_state_activate_iclog(
> + struct xlog_in_core *iclog,
> + int *iclogs_changed)
> +{
> + ASSERT(list_empty_careful(&iclog->ic_callbacks));
> +
> + /*
> + * If the number of ops in this iclog indicate it just contains the
> + * dummy transaction, we can change state into IDLE (the second time
> + * around). Otherwise we should change the state into NEED a dummy.
> + * We don't need to cover the dummy.
> + */
> + if (*iclogs_changed == 0 &&
> + iclog->ic_header.h_num_logops == cpu_to_be32(XLOG_COVER_OPS)) {
> + *iclogs_changed = 1;
> + } else {
> + /*
> + * We have two dirty iclogs so start over. This could also be
> + * num of ops indicating this is not the dummy going out.
> + */
> + *iclogs_changed = 2;
> + }
> +
> + iclog->ic_state = XLOG_STATE_ACTIVE;
> + iclog->ic_offset = 0;
> + iclog->ic_header.h_num_logops = 0;
> + memset(iclog->ic_header.h_cycle_data, 0,
> + sizeof(iclog->ic_header.h_cycle_data));
> + iclog->ic_header.h_lsn = 0;
> +}
> +
> /*
> - * An iclog has just finished IO completion processing, so we need to update
> - * the iclog state and propagate that up into the overall log state. Hence we
> - * prepare the iclog for cleaning, and then clean all the pending dirty iclogs
> - * starting from the head, and then wake up any threads that are waiting for the
> - * iclog to be marked clean.
> - *
> - * The ordering of marking iclogs ACTIVE must be maintained, so an iclog
> - * doesn't become ACTIVE beyond one that is SYNCING. This is also required to
> - * maintain the notion that we use a ordered wait queue to hold off would be
> - * writers to the log when every iclog is trying to sync to disk.
> - *
> - * Caller must hold the icloglock before calling us.
> - *
> - * State Change: !IOERROR -> DIRTY -> ACTIVE
> + * Loop through all iclogs and mark all iclogs currently marked DIRTY as
> + * ACTIVE after iclog I/O has completed.
> */
> -STATIC void
> -xlog_state_clean_iclog(
> +static void
> +xlog_state_activate_iclogs(
> struct xlog *log,
> - struct xlog_in_core *dirty_iclog)
> + int *iclogs_changed)
> {
> - struct xlog_in_core *iclog;
> - int changed = 0;
> -
> - /* Prepare the completed iclog. */
> - if (dirty_iclog->ic_state != XLOG_STATE_IOERROR)
> - dirty_iclog->ic_state = XLOG_STATE_DIRTY;
> + struct xlog_in_core *iclog = log->l_iclog;
>
> - /* Walk all the iclogs to update the ordered active state. */
> - iclog = log->l_iclog;
> do {
> - if (iclog->ic_state == XLOG_STATE_DIRTY) {
> - iclog->ic_state = XLOG_STATE_ACTIVE;
> - iclog->ic_offset = 0;
> - ASSERT(list_empty_careful(&iclog->ic_callbacks));
> - /*
> - * If the number of ops in this iclog indicate it just
> - * contains the dummy transaction, we can
> - * change state into IDLE (the second time around).
> - * Otherwise we should change the state into
> - * NEED a dummy.
> - * We don't need to cover the dummy.
> - */
> - if (!changed &&
> - (be32_to_cpu(iclog->ic_header.h_num_logops) ==
> - XLOG_COVER_OPS)) {
> - changed = 1;
> - } else {
> - /*
> - * We have two dirty iclogs so start over
> - * This could also be num of ops indicates
> - * this is not the dummy going out.
> - */
> - changed = 2;
> - }
> - iclog->ic_header.h_num_logops = 0;
> - memset(iclog->ic_header.h_cycle_data, 0,
> - sizeof(iclog->ic_header.h_cycle_data));
> - iclog->ic_header.h_lsn = 0;
> - } else if (iclog->ic_state == XLOG_STATE_ACTIVE)
> - /* do nothing */;
> - else
> - break; /* stop cleaning */
> - iclog = iclog->ic_next;
> - } while (iclog != log->l_iclog);
> -
> + if (iclog->ic_state == XLOG_STATE_DIRTY)
> + xlog_state_activate_iclog(iclog, iclogs_changed);
> + /*
> + * The ordering of marking iclogs ACTIVE must be maintained, so
> + * an iclog doesn't become ACTIVE beyond one that is SYNCING.
> + */
> + else if (iclog->ic_state != XLOG_STATE_ACTIVE)
> + break;
> + } while ((iclog = iclog->ic_next) != log->l_iclog);
> +}
>
> +static int
> +xlog_covered_state(
> + int prev_state,
> + int iclogs_changed)
> +{
> /*
> - * Wake up threads waiting in xfs_log_force() for the dirty iclog
> - * to be cleaned.
> + * We usually go to NEED. But we go to NEED2 if the changed indicates we
> + * are done writing the dummy record. If we are done with the second
> + * dummy recored (DONE2), then we go to IDLE.
> */
> - wake_up_all(&dirty_iclog->ic_force_wait);
> + switch (prev_state) {
> + case XLOG_STATE_COVER_IDLE:
> + case XLOG_STATE_COVER_NEED:
> + case XLOG_STATE_COVER_NEED2:
> + break;
> + case XLOG_STATE_COVER_DONE:
> + if (iclogs_changed == 1)
> + return XLOG_STATE_COVER_NEED2;
> + break;
> + case XLOG_STATE_COVER_DONE2:
> + if (iclogs_changed == 1)
> + return XLOG_STATE_COVER_IDLE;
> + break;
> + default:
> + ASSERT(0);
> + }
>
> - /*
> - * Change state for the dummy log recording.
> - * We usually go to NEED. But we go to NEED2 if the changed indicates
> - * we are done writing the dummy record.
> - * If we are done with the second dummy recored (DONE2), then
> - * we go to IDLE.
> - */
> - if (changed) {
> - switch (log->l_covered_state) {
> - case XLOG_STATE_COVER_IDLE:
> - case XLOG_STATE_COVER_NEED:
> - case XLOG_STATE_COVER_NEED2:
> - log->l_covered_state = XLOG_STATE_COVER_NEED;
> - break;
> + return XLOG_STATE_COVER_NEED;
> +}
>
> - case XLOG_STATE_COVER_DONE:
> - if (changed == 1)
> - log->l_covered_state = XLOG_STATE_COVER_NEED2;
> - else
> - log->l_covered_state = XLOG_STATE_COVER_NEED;
> - break;
> +STATIC void
> +xlog_state_clean_iclog(
> + struct xlog *log,
> + struct xlog_in_core *dirty_iclog)
> +{
> + int iclogs_changed = 0;
>
> - case XLOG_STATE_COVER_DONE2:
> - if (changed == 1)
> - log->l_covered_state = XLOG_STATE_COVER_IDLE;
> - else
> - log->l_covered_state = XLOG_STATE_COVER_NEED;
> - break;
> + if (dirty_iclog->ic_state != XLOG_STATE_IOERROR)
> + dirty_iclog->ic_state = XLOG_STATE_DIRTY;
>
> - default:
> - ASSERT(0);
> - }
> + xlog_state_activate_iclogs(log, &iclogs_changed);
> + wake_up_all(&dirty_iclog->ic_force_wait);
> +
> + if (iclogs_changed) {
> + log->l_covered_state = xlog_covered_state(log->l_covered_state,
> + iclogs_changed);
> }
> }
>
> --
> 2.25.1
>
next prev parent reply other threads:[~2020-03-20 11:52 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-20 6:53 more log cleanups Christoph Hellwig
2020-03-20 6:53 ` [PATCH 1/8] xfs: merge xlog_cil_push into xlog_cil_push_work Christoph Hellwig
2020-03-20 6:53 ` [PATCH 2/8] xfs: factor out a xlog_wait_on_iclog helper Christoph Hellwig
2020-03-20 6:53 ` [PATCH 3/8] xfs: simplify the xfs_log_release_iclog calling convention Christoph Hellwig
2020-03-20 6:53 ` [PATCH 4/8] xfs: simplify log shutdown checking in xfs_log_release_iclog Christoph Hellwig
2020-03-20 6:53 ` [PATCH 5/8] xfs: remove the aborted parameter to xlog_state_done_syncing Christoph Hellwig
2020-03-23 15:27 ` Darrick J. Wong
2020-03-20 6:53 ` [PATCH 6/8] xfs: refactor xlog_state_clean_iclog Christoph Hellwig
2020-03-20 11:51 ` Brian Foster [this message]
2020-03-20 6:53 ` [PATCH 7/8] xfs: move the ioerror check out of xlog_state_clean_iclog Christoph Hellwig
2020-03-20 6:53 ` [PATCH 8/8] xfs: remove xlog_state_want_sync 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=20200320115151.GA42814@bfoster \
--to=bfoster@redhat.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;
as well as URLs for NNTP newsgroup(s).