Re: [PATCH 5/5] bcachefs: use prejournaled key updates for write buffer flushes

[Brian Foster <bfoster@redhat.com>]

On Thu, Jul 20, 2023 at 05:26:57PM -0400, Kent Overstreet wrote:
> On Wed, Jul 19, 2023 at 08:53:06AM -0400, Brian Foster wrote:
> > The write buffer mechanism journals keys twice in certain
> > situations. A key is always journaled on write buffer insertion, and
> > is potentially journaled again if a write buffer flush falls into
> > either of the slow btree insert paths. This has shown to cause
> > journal recovery ordering problems in the event of an untimely
> > crash.
> > 
> > For example, consider if a key is inserted into index 0 of a write
> > buffer, the active write buffer switches to index 1, the key is
> > deleted in index 1, and then index 0 is flushed. If the original key
> > is rejournaled in the btree update from the index 0 flush, the (now
> > deleted) key is journaled in a seq buffer ahead of the latest
> > version of key (which was journaled when the key was deleted in
> > index 1). If the fs crashes while this is still observable in the
> > log, recovery sees the key from the btree update after the delete
> > key from the write buffer insert, which is the incorrect order. This
> > problem is occasionally reproduced by generic/388 and generally
> > manifests as one or more backpointer entry inconsistencies.
> > 
> > To avoid this problem, never rejournal write buffered key updates to
> > the associated btree. Instead, use prejournaled key updates to pass
> > the journal seq of the write buffer insert down to the btree insert,
> > which updates the btree leaf pin to reflect the seq of the key.
> > 
> > Note that tracking the seq is required instead of just using
> > NOJOURNAL here because otherwise we lose protection of the write
> > buffer pin when the buffer is flushed, which means the key can fall
> > off the tail of the on-disk journal before the btree leaf is flushed
> > and lead to similar recovery inconsistencies.
> 
> Fairly simple, and it looks like this fixes all the backpointers errors
> - nice, applied.
> 

Thanks!

> So journal replay does something similar, where we're doing a btree
> update for a key that already lives in a particular journal entry and
> the dependency needs to be recorded. Could you have a quick look to see
> if there's any cleanups/unification worth doing there? Or if not, just
> leave a note.
> 

Interesting. IIRC, one of the purposes of generic/388 was to test
crashes during recovery itself, so we should have at least some coverage
in that area.

Can you point to an example or area of code where this occurs with
recovery? (Or do you mean recovery in general has this rejournaling
behavior?)

> Also, did you look at adding the tracepoint we talked about, for when
> we're doing btree updates that are already at the end of the journal and
> immediately need to be flushed?
> 

Not yet.. I wanted to get this worked out first and then ran into some
sort of contention issue during some fstests that I had to dig into
before I got a chance to think about it. I'll send a separate mail on
that one and will probably have to pick up both once I'm back..

Brian