Re: fstests generic/441 -- occasional bcachefs failure

[Kent Overstreet <kent.overstreet@linux.dev>]

On Tue, Jan 31, 2023 at 11:04:11AM -0500, Brian Foster wrote:
> 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.

If you link me your git repo I'll add it to the CI so all the automated
tests run.

> 
> > 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()).

*nod* That's more accurate.

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

Possibly. AFAIK, on typical block devices we shouldn't see write errors
unless the device is really not working anymore - both spinning rust and
SSDs will remap on write error, and on transport error IMO the block
layer is the more correct place to be issuing retries, if so desired.

But we do have aspirations of running on raw flash in the
not-too-distant future (ZNS SSDs), and I would expect we'll need retry
logic at that point.

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

I'd think we'd want a timeout, not a specific number of retries to
attempt (or perhaps both).

> 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?

Freeze definitely needs to happen. It's been _ages_ since I was looking
at it so I couldn't say offhand where we'd need to start, but if you're
interested I'd be happy to look at what it'd take.

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

Yes - bch2_btree_key_cache_journal_flush() -> btree_key_cache_flush_pos()
-> bch2_trans_commit().

> 
> >  - 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.

Wherever you read that must date to well before bcachefs was a thing -
that was true in bcache when the journalling code was still new. In
bcachefs ordering of updates is important and the journal is indeed our
global time ordering.

> 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..?

Correct.

> (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.).

I see -  in XFS metadata items can't be written until the relevant log
entry is written? Yeah we never did it that way in bcachefs, blocking
writeback which already blocks memory reclaim sounded like a bad idea :)

Originally, before we started updating pointers to btree nodes after
every write, we'd note in every btree node write the newest journal
sequence number it had updates for - then in recovery we'd blacklist the
next N journal sequence numbers and if we ever saw a btree node entry
for a blacklisted journal sequence number we just ignored it.

> #bcache or #bachefs?

#bcache :)