From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 2/8] xfs: don't wait on future iclogs when pushing the CIL
Date: Fri, 18 Jun 2021 07:55:15 +1000 [thread overview]
Message-ID: <20210617215515.GB664593@dread.disaster.area> (raw)
In-Reply-To: <20210617174910.GT158209@locust>
On Thu, Jun 17, 2021 at 10:49:10AM -0700, Darrick J. Wong wrote:
> On Thu, Jun 17, 2021 at 06:26:11PM +1000, Dave Chinner wrote:
> > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> > index 705619e9dab4..2fb0ab02dda3 100644
> > --- a/fs/xfs/xfs_log_cil.c
> > +++ b/fs/xfs/xfs_log_cil.c
> > @@ -1075,15 +1075,54 @@ xlog_cil_push_work(
> > ticket = ctx->ticket;
> >
> > /*
> > - * If the checkpoint spans multiple iclogs, wait for all previous
> > - * iclogs to complete before we submit the commit_iclog. In this case,
> > - * the commit_iclog write needs to issue a pre-flush so that the
> > - * ordering is correctly preserved down to stable storage.
> > + * If the checkpoint spans multiple iclogs, wait for all previous iclogs
> > + * to complete before we submit the commit_iclog. We can't use state
> > + * checks for this - ACTIVE can be either a past completed iclog or a
> > + * future iclog being filled, while WANT_SYNC through SYNC_DONE can be a
> > + * past or future iclog awaiting IO or ordered IO completion to be run.
> > + * In the latter case, if it's a future iclog and we wait on it, the we
> > + * will hang because it won't get processed through to ic_force_wait
> > + * wakeup until this commit_iclog is written to disk. Hence we use the
> > + * iclog header lsn and compare it to the commit lsn to determine if we
> > + * need to wait on iclogs or not.
> > */
> > spin_lock(&log->l_icloglock);
> > if (ctx->start_lsn != commit_lsn) {
> > - xlog_wait_on_iclog(commit_iclog->ic_prev);
> > - spin_lock(&log->l_icloglock);
> > + struct xlog_in_core *iclog;
> > +
> > + for (iclog = commit_iclog->ic_prev;
> > + iclog != commit_iclog;
> > + iclog = iclog->ic_prev) {
> > + xfs_lsn_t hlsn;
> > +
> > + /*
> > + * If the LSN of the iclog is zero or in the future it
> > + * means it has passed through IO completion and
> > + * activation and hence all previous iclogs have also
> > + * done so. We do not need to wait at all in this case.
> > + */
> > + hlsn = be64_to_cpu(iclog->ic_header.h_lsn);
> > + if (!hlsn || XFS_LSN_CMP(hlsn, commit_lsn) > 0)
> > + break;
> > +
> > + /*
> > + * If the LSN of the iclog is older than the commit lsn,
> > + * we have to wait on it. Waiting on this via the
> > + * ic_force_wait should also order the completion of all
> > + * older iclogs, too, but we leave checking that to the
> > + * next loop iteration.
> > + */
> > + ASSERT(XFS_LSN_CMP(hlsn, commit_lsn) < 0);
> > + xlog_wait_on_iclog(iclog);
> > + spin_lock(&log->l_icloglock);
>
> The presence of a loop here confuses me a bit -- we really only need to
> check and wait on commit->ic_prev since xlog_wait_on_iclog waits for
> both the iclog that it is given as well as all previous iclogs, right?
I originally wrote this thinking about using the ic_write_wait queue
which would require checking all iclogs in the ring because the
completion signalled at the DONE_SYNC state is not ordered against
other iclogs. Hence I had planned to walk all the iclogs. THen I
realised that checking the LSN could tell us past/future and so we
only needed to wait on the first iclog with a LSN less than the
commit iclog.
ANd so I left the loop in place to ensure that, even if my assertion
about the ring aging order was incorrect, this code would Do The
Right Thing.
> we've waited on commit->ic_prev, the next iclog iterated (i.e.
> commit->ic_prev->ic_prev) should break out of the loop?
Yes, that is what it does.
I can strip this all out - it was really just being defensive
because I wanted to make sure things were working as I expected them
to be working...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2021-06-17 21:55 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-17 8:26 [PATCH 0/8 V2] xfs: log fixes for for-next Dave Chinner
2021-06-17 8:26 ` [PATCH 1/8] xfs: add iclog state trace events Dave Chinner
2021-06-17 16:45 ` Darrick J. Wong
2021-06-18 14:09 ` Christoph Hellwig
2021-06-17 8:26 ` [PATCH 2/8] xfs: don't wait on future iclogs when pushing the CIL Dave Chinner
2021-06-17 17:49 ` Darrick J. Wong
2021-06-17 21:55 ` Dave Chinner [this message]
2021-06-17 8:26 ` [PATCH 3/8] xfs: move xlog_commit_record to xfs_log_cil.c Dave Chinner
2021-06-17 12:57 ` kernel test robot
2021-06-17 12:57 ` kernel test robot
2021-06-17 17:50 ` Darrick J. Wong
2021-06-17 21:56 ` Dave Chinner
2021-06-18 14:16 ` Christoph Hellwig
2021-06-17 8:26 ` [PATCH 4/8] xfs: pass a CIL context to xlog_write() Dave Chinner
2021-06-17 14:46 ` kernel test robot
2021-06-17 14:46 ` kernel test robot
2021-06-17 20:24 ` Darrick J. Wong
2021-06-17 22:03 ` Dave Chinner
2021-06-17 22:18 ` Darrick J. Wong
2021-06-18 14:23 ` Christoph Hellwig
2021-06-17 8:26 ` [PATCH 5/8] xfs: factor out log write ordering from xlog_cil_push_work() Dave Chinner
2021-06-17 19:59 ` Darrick J. Wong
2021-06-18 14:27 ` Christoph Hellwig
2021-06-18 22:34 ` Dave Chinner
2021-06-17 8:26 ` [PATCH 6/8] xfs: separate out setting CIL context LSNs from xlog_write Dave Chinner
2021-06-17 20:28 ` Darrick J. Wong
2021-06-17 22:10 ` Dave Chinner
2021-06-17 8:26 ` [PATCH 7/8] xfs: attached iclog callbacks in xlog_cil_set_ctx_write_state() Dave Chinner
2021-06-17 20:55 ` Darrick J. Wong
2021-06-17 22:20 ` Dave Chinner
2021-06-17 8:26 ` [PATCH 8/8] xfs: order CIL checkpoint start records Dave Chinner
2021-06-17 21:31 ` Darrick J. Wong
2021-06-17 22:49 ` Dave Chinner
2021-06-17 18:32 ` [PATCH 0/8 V2] xfs: log fixes for for-next Brian Foster
2021-06-17 19:05 ` Darrick J. Wong
2021-06-17 20:06 ` Brian Foster
2021-06-17 20:26 ` Darrick J. Wong
2021-06-17 23:31 ` Brian Foster
2021-06-17 23:43 ` Dave Chinner
2021-06-18 13:08 ` Brian Foster
2021-06-18 13:55 ` Christoph Hellwig
2021-06-18 14:02 ` Christoph Hellwig
2021-06-18 22:28 ` Dave Chinner
2021-06-18 22:15 ` Dave Chinner
2021-06-18 22:48 ` Dave Chinner
2021-06-19 20:22 ` Darrick J. Wong
2021-06-20 22:18 ` Dave Chinner
-- strict thread matches above, loose matches on Subject: below --
2021-06-26 23:10 [PATCH 4/8] xfs: pass a CIL context to xlog_write() kernel test robot
2021-06-28 8:58 ` Dan Carpenter
2021-06-28 8:58 ` Dan Carpenter
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=20210617215515.GB664593@dread.disaster.area \
--to=david@fromorbit.com \
--cc=djwong@kernel.org \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.