Re: fstests generic/441 -- occasional bcachefs failure

[Brian Foster <bfoster@redhat.com>]

On Mon, Jan 30, 2023 at 12:06:34PM -0500, Kent Overstreet wrote:
> On Fri, Jan 27, 2023 at 09:50:05AM -0500, Brian Foster wrote:
> > Something else that occurred to me while looking further at this is we
> > can also avoid the error in this case fairly easily by bailing out of
> > bch2_fsync() if page writeback fails, as opposed to the unconditional
> > flush -> sync meta -> flush log sequence that returns the first error
> > anyways. That would prevent marking the inode with a new sequence number
> > when I/Os are obviously failing. The caveat is that the test still
> > fails, now with a "Read-only file system" error instead of EIO, because
> > the filesystem is shutdown by the time the vfs write inode path actually
> > runs.
> 
> If some pages did write successfully we don't want to skip the rest of
> the fsync, though.
> 

What does it matter if the fsync() has already failed? ISTM this is
pretty standard error handling behavior across major fs', but it's not
clear to me if there's some bcachefs specific quirk that warrants
different handling..

FWIW, I think I've been able to fix this test with a couple small
tweaks:

1. Change bch2_fsync() to return on first error.
2. Introduce a bcachefs specific delay in the working -> error table
switchover.

I've not run a full regression test on the bcachefs change yet, but if
you're willing to take a patch along those lines I can do that and then
if it survives, follow up with an upstream fstests patch. That would
allow this test to pass fairly reliably and also no longer
unconditionally shutdown bcachefs.

> The test failing because the filesystem went read-only is because a
> btree node write fails - I'm assuming without that change the test just
> fails before any btree node writes are attempted.
> 

Not sure I parse the above. Generally speaking, the test fails because
after the last successful fsync, it immediately switches over to the
error dm table. The fs still has some reclaim/journal work to do after
this point, however, so this essentially always results in emergency
shutdown. Whether the test passes or fails at this point depends on a
race between this shutdown sequence in the background and an fsync call
successfuly committing a transaction (calling bch2_mark_inode()).

The winner of the race dictates whether the first fsync() after
switching back to the working dm table can avoid the journal flush
because the seq associated with the inode is already on disk, or whether
the fs wants to flush the seq on a filesystem that has been shutdown
(and so will always fail).

> > All in all ISTM the shutdown is the fundamental issue wrt to this test.
> > I'm not sure it's worth complicating the log flush error handling with
> > errseq and whatnot just to filter out certain errors when the fs has
> > seen a catastrophic failure/shutdown (also agree with your followup that
> > isn't the most critical bug in the world anyways).
> 
> Well, our default setting on serious error is to go read-only, an a
> btree node write failing (in a single device, non replicated filesystem)
> definitely qualifies.
> 
> It looks like our behaviour on btree node write error does need to be
> looked at and improved/fixed: btree_io.c line 1734 has a clearly
> unfinished case. It looks like we're missing whatever logic should be
> there for a replicated filesystem - we definitely need to decide what to
> do when some but not all btree node writes fail.
> 

Interesting. I noticed that hunk and thought it looked odd, but didn't
have enough context to grok it.

> More relevant to this test, we also a setting for error behaviour -
> errors=(continue|ro|panic), but that explicitly does _not_ apply to
> write IO errors; if we can't write out metadata we always go read-only.
> 
> I'm not in a hurry to change that, because one of the things bcachefs
> currently does really well is stopping the world if things ever go so
> wrong that the filesystem would be inconsistent - we haven't had too
> much in the way of unrecoverable filesystems, and I don't want that to
> change :)
> 

Indeed, that makes sense. I wonder if it would make sense to issue
retries at some point?

As another XFS example, it also has an emergency shutdown mechanism for
similar purposes. Certain I/O failures result in immediate shutdown of
the fs and no further changes make it to disk. Because shutdown itself
is a catastrophic failure, metadata write failures are retried at least
once before initiating shutdown to filter out transient errors. On top
of that, there's a sysfs section for each mount that allows an admin to
configure error handling behavior as needed. This allows setting the
number of retries before an error is considered permanent (including
infinite retries for scenarios where errors are expected to be manually
recoverable), or how long to delay retries.

I'm not sure a simple retry once sequence would help with this test, but
an infinite retry option (with some delay) probably would.

OTOH, I wonder if a more elegant solution is to explicitly quiesce the
fs before the working -> error table switchover. That might typically be
done with a freeze/unfreeze cycle so the test can serialize against
whatever background work needs to complete before putting the block
device into error mode. However, I notice freeze isn't currently
supported on bcachefs. Any thoughts on freeze support?

> > Either way, I am trying to grok the commit/writeback/journaling path a
> > bit better here to try and understand why the fs shuts down in this test
> > whereas other fs' don't seem to. One thing I'm noticing is that this
> > whole path is rather asynchronous, making it a bit hard to follow. Any
> > tips on the best way to work through the big picture steps involved in
> > the path from a trans commit, through journaling, to eventual btree
> > writeback?
> 
> Transaction commit path is pretty much all straight line synchronous
> code - that's btree_update_leaf.c.
> 
> Basically, the transaction commit path takes a list of updates, and
>  - runs triggers (which will then generate more updates!)
>  - ensures all btree paths are fully traversed and have intent locks
>  - takes write locks on btree nodes
>  - gets a journal reservation
>  - does updates to leaf nodes/btree key cache (and soon the btree write
>    buffer as well)
> 

Yep, I was able to follow most of this with the exception of some
confusion over the key cache bits. IIUC, the initial trans commit might
add a key update to the key cache, whereas reclaim might commit another
transaction to flush the key cache to the btree (via the journal
callback you mention below).

> Btree node merging and splitting is also driven by the transaction
> commit path - before doing the update we check if we need to merge
> btree nodes (it has to be before the transaction commit in order to
> happen reliably), and btree node splitting is done in an error path -
> after taking write locks if we don't have space for the insert we bail
> out and to the split in bch2_trans_commit_error().
>  
> When we update a leaf node or btree key cache key, we mark it as dirty
> and create a journal pin for the journal sequence number we got a
> journal reservation for. A journal pin has a callback for journal
> reclaim to invoke - typically, writeback for the btree node cache and
> btree key cache is driven entirely by journal reclaim, and the journal
> pin also keeps the relevant journal entry dirty so that it'll be
> replayed by journal replay.
> 
> (That writeback behaviour will be changing at some point: the behaviour
> up until recently was that journal reclaim monitored the dirty
> percentage of the btree node cache and key cache and would run to those
> numbers under some threshold, but it turns out that really hurts
> performance on purely random write benchmarks - we writeback too
> aggressively, and we need to allow those caches to be 100% dirty if
> there isn't any memory pressure).
> 
> The journalling code is pretty self contained w.r.t. the rest of
> bcachefs. It is indeed a big pile of tricky asynchronous code (the fast
> paths need to be lockless - right now I'm working on making it even more
> lockless - and there's a lot of pipelining it has to do). At a high
> level it's pretty simple though, and you should be able to treat it as a
> black box. The main operations are:
> 
>  - getting a reservation: this gets you a reservation in a specific
>    journal entry - you're supposed to memcpy() whatever you want
>    journalled into the entry to be written and then quickly drop your
>    reservation.
> 
>    Blocking with a reservation will block the entire journal - note that
>    we get our journal reservation after taking btree write locks in the
>    transaction commit path (but we do it in nonblocking mode; if we have
>    to block on the journal we drop btree locks and do the waiting in
>    bch2_trans_commit_error()).
>  
>  - getting a pre-reservation: trying to get a journal reservation while
>    holding a journal pin would deadlock the journal if it ever
>    completely filled up - a pre-reservation allows reserving space in
>    the journal so that we can get a reservation later without
>    deadlocking. This is for e.g. flushing the btree key cache: when we
>    do a btree key cache update we update the key cache in memory and
>    journal the update, to flush the key cache we have to update the
>    btree which requires (for simplicity) re-journalling the update.
> 

Ah.. so does "for simplicity" here essentially mean "for reuse of the
transaction code?"

>  - flush_seq: given a journal sequence number we previously got a
>    journal reservation on, request that it be written out (with a
>    flush/fua write, not all journal entries are written flush/fua).
> 

Thanks for this. Something that initially confused me wrt to the journal
is that I was seeing journal writes after btree node writes. The docs
mention the journal is primarily an optimization and ordering is not
important for crash consistency. With that in mind, does that mean a
journal "pin" refers to entries held in the journal until written back
to the btree (as opposed to the journal pinning keys in memory)? IOW, it
doesn't really matter in which order that journal/btree writes occur,
but only that entries remain pinned in the journal until btree writes
complete..?

(I suspect this confused me because XFS uses the same pinning
terminology, but in that context it refers to metadata items being
pinned by the log, until the log item is flushed and thus unpins the
metadata item for final writeback. So ordering is a critical factor in
this context.).

> Then there's btree node writes, which it sounds like you might be
> getting into soon. Btree node writes are more complicated: btree nodes
> are log structured, so we have to differentiate between the first write
> to a new node and subsequent appending writes, and before returning a
> completion for an appending btree node write we have to update the
> pointer to that btree node (btree node pointers record the number of
> sectors currently written; this is important for multi device
> filesystems, and also for ensuring consistent ordering of updates after
> a crash - updates can't be visible if they weren't completed in the
> journal, so we have to ignore btree node writes that are newer than the
> newest completed journal write.
> 
> Originally this was handled with the journal sequence number blacklist
> mechanism - so you might see a few references to that still in
> btree_io.c - but now it's handled by updating parent pointers after
> every write, up to the root.
> 
> Hope that helps - and feel free to ask me more questions on IRC,
> irc.oftc.net#bcache.
> 

#bcache or #bachefs?

This is all extremely helpful. Thanks again.

Brian

> Cheers,
> Kent
>