Re: [PATCH 2/2] xfs: Throttle commits on delayed background CIL push

From: Dave Chinner <david@fromorbit.com>
To: Brian Foster <bfoster@redhat.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 2/2] xfs: Throttle commits on delayed background CIL push
Date: Tue, 8 Oct 2019 13:51:57 +1100	[thread overview]
Message-ID: <20191008025157.GE16973@dread.disaster.area> (raw)
In-Reply-To: <20191004115001.GA6706@bfoster>

On Fri, Oct 04, 2019 at 07:50:01AM -0400, Brian Foster wrote:
> On Fri, Oct 04, 2019 at 12:27:55PM +1000, Dave Chinner wrote:
> > On Thu, Oct 03, 2019 at 10:41:14AM -0400, Brian Foster wrote:
> > > Hmm, I'm also not sure the lockless reservation algorithm is totally
> > > immune to increased concurrency in this regard. What prevents multiple
> > > tasks from racing through xlog_grant_head_check() and blowing past the
> > > log head, for example?
> > 
> > Nothing. Debug kernels even emit a "xlog_verify_grant_tail: space >
> > BBTOB(tail_blocks)" messages when that happens. It's pretty
> > difficult to do this in real world conditions, even when there is
> > lots of concurrency being used.
> > 
> 
> Hm, Ok. Though I've seen that alert enough times that I
> (unintentionally) ignore it at this point, so it can't be that hard to
> reproduce. ;) That is usually during fstests however, and not a typical
> workload that I recall.

I can't say I've seen it for a long time now - I want to say "years"
but I may well have simply missed it on the rare occasion it has
occurred and fstests hasn't captured it. i.e. fstests is supposed to
capture unusual things like this appearing in dmesg during a
test....

> Of course, there's a difference between
> reproducing the basic condition and taking it to the point where it
> manifests into a problem.

*nod*

> > But here's the rub: it's not actually the end of the world because
> > the reservation doesn't actually determine how much of the log is
> > currently being used by running transactions - the reservation is
> > for a maximal rolling iteration of a permanent transaction, not the
> > initial transaction will be running. Hence if we overrun
> > occassionally we don't immediately run out of log space and corrupt
> > the log.
> > 
> 
> Ok, that much is evident from the amount of time this mechanism has been
> in place without any notable issues.
> 
> > Yes, if none of the rolling transactions complete and they all need
> > to use their entire reservation, and the tail of the log cannot be
> > moved forward because it is pinned by one of the transactions that
> > is running, then we'll likely get a log hang on a regrant on the
> > write head. But if any of the transactions don't use all of their
> > reservation, then the overrun gets soaked up by the unused parts of
> > the transactions that are completed and returned to reservation
> > head, and nobody even notices taht there was a temporary overrun of
> > the grant head space.
> > 
> 
> Ok, I didn't expect this to be some catastrophic problem or really a
> problem with your patch simply based on the lifetime of the code and how
> the grant heads are actually used. I was going to suggest an assert or
> something to detect whether batching behavior as a side effect of the
> commit throttle would ever increase likelihood of this situation, but it
> looks like the grant verify function somewhat serves that purpose
> already.

Yeah - xlog_verify_grant_tail() will the report reservation
overruns, but the serious log space problems (i.e. head overwritting
the tail) are detected by xlog_verify_tail_lsn() when we stamp the
tail_lsn into the current iclog header. That's still done under the
icloglock and the AIL lock, so the comparison of the tail with the
current log head is still completely serialised.

> I'd _prefer_ to see something, at least in DEBUG mode, that indicates
> the frequency of the fundamental incorrect accounting condition as
> opposed to just the side effect of blowing the tail (because the latter
> depends on other difficult to reproduce factors), but I'd have to think
> about that some more as it would need to balance against normal/expected
> execution flow. Thanks for the background.

You can test that just by removing the XLOG_TAIL_WARN flag setting,
then it will warn on every reservation overrun rather than just the
first.

> > Hence occasional overruns on the reservation head before they start
> > blocking isn't really a problem in practice because the probability
> > of all the transaction reservation of all transactions running being
> > required to make forwards progress is extremely small.
> > 
> > Basically, we gave up "perfect reservation space grant accounting"
> > because performance was extremely important and risk of log hangs as
> > a result of overruns was considered to be extremely low and worth
> > taking for the benefits the algorithm provided. This was just a
> > simple, pragmatic risk based engineering decision.
> > 
> 
> FWIW, the comment for xlog_verify_tail() also suggests the potential for
> false positives and references a panic tag, which all seems kind of
> erratic and misleading compared to what you explain here.

Well, it's fundamentally an unserialised check, so it can race with
other reservation grants, commits that release grant space and tail
lsn updates. Hence it's not a 100% reliable debug check.

It also used to be run at all times, not just under
XFS_CONFIG_DEBUG=y, which is why it has a panic tag associated with
it. When we first deployed it, we weren't 100% sure there weren't
customer workloads that would trip over this and hang the log, so
we gave ourselves a way of triggering kernel dumps the instant an
overrun was detected. Hence a site that had log hangs with this
message in the logs could turn on the panic tag and we'd get a
kernel dump to analyse...

Since then, this code has been relegated to debug code but the panic
tag still exists. It could be turned back into a ASSERT now, but
it's still useful the way it is as it means debug kernels don't fall
over the moment a spurious overrun occurs...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com