Re: [PATCH 3/3] bcachefs: fix race between journal entry close and pin set

[Brian Foster <bfoster@redhat.com>]

On Sun, Sep 03, 2023 at 10:29:28PM -0400, Kent Overstreet wrote:
> On Sun, Sep 03, 2023 at 05:18:12PM -0400, Kent Overstreet wrote:
> > On Thu, Aug 31, 2023 at 07:07:34AM -0400, Brian Foster wrote:
> > > bcachefs freeze testing via fstests generic/390 occasionally
> > > reproduces the following BUG from bch2_fs_read_only():
> > > 
> > >   BUG_ON(atomic_long_read(&c->btree_key_cache.nr_dirty));
> >
> > Your fix makes sense, but I wonder if it might be simpler and better to
> > fix this in bch2_journal_reclaim_fast().
> 
> What do you think of this fix?
> 
> diff --git a/fs/bcachefs/journal_reclaim.c b/fs/bcachefs/journal_reclaim.c
> index 10e1860dad..021c56ac67 100644
> --- a/fs/bcachefs/journal_reclaim.c
> +++ b/fs/bcachefs/journal_reclaim.c
> @@ -303,6 +303,12 @@ static void bch2_journal_reclaim_fast(struct journal *j)
>  	 */
>  	while (!fifo_empty(&j->pin) &&
>  	       !atomic_read(&fifo_peek_front(&j->pin).count)) {
> +		u64 seq = journal_last_seq(j);
> +
> +		if (seq > j->seq_ondisk &&
> +		    journal_state_count(j->reservations, seq & JOURNAL_BUF_MASK))
> +			break;
> +
>  		fifo_pop(&j->pin, temp);
>  		popped = true;
>  	}
> 

So my first reaction once I figured out what was going on was to
consider something like this, because it seemed like the issue was
premature release of the journal tail. After digging into that a bit it
didn't quite feel right, which is what had me digging further into why
the ref count was zero on a seq with outstanding reservation.

I think I see what the above is doing, but I really don't see how this
is simplified in any way. Maybe the code changes are slightly smaller,
but IMO the bigger picture is far easier to reason about when the
reference counts are properly honored. I.e., the outstanding
reservations hold references on the journal buffer associated with the
seq, and the active journal buffer holds a reference on the pin fifo.

Conversely, the above strikes me as unnecessarily clever based on the
fact that just to review it I need to grok behavior in several different
contexts (res and seq management, on disk seq updates, etc.) to convince
myself whether it's safe, and even then I'm not totally sure I have
enough context to say whether the state count check can't ever break on
something like a reused (for a new seq) journal buffer. Heh, the more I
think about that the more I have to ask: why not just make the reference
count work? ;P

FWIW, one thing I disliked about the original patch was the need to
mostly duplicate the buf put helper due to the locking context quirk. I
was trying to avoid that, but I ran out of ideas before I wanted to move
on actually fixing the bug. My preference would be to address the
reference counting issue as is (to preserve design simplicity), and then
maybe think a bit harder about cleaning up the res put implementation if
the primary concern is that we feel like this starts to make things a
bit convoluted..

Brian