On Sat, Sep 09, 2023 at 10:03:04PM -0400, Kent Overstreet wrote: > On Wed, Sep 06, 2023 at 03:07:58PM -0400, Brian Foster wrote: > > On Tue, Sep 05, 2023 at 08:59:02AM -0400, Brian Foster wrote: > > > 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(). > > ... > > > > > > 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.. > > > > > > > Another thought that comes to mind is to perhaps allow the journal_res > > to hold a reference to the pin fifo for the associated seq.. The idea > > would be we could continue to hold a reference during the open/close > > journal buffer lifecycle, but a res of the same seq would acquire an > > additional reference as well to keep the tail from popping before a > > transaction can actually set a pin (i.e. essentially parallel to the > > buffer reference). > > Yeah that would probably be cleanest - but it would be much too heavy. >
I'm curious how so? Additional atomics? If so, we could also consider something that allows a pin list ref on the reservation to simply transfer to a pin set via the transaction. That might not add any new overhead over the current code, but would require some plumbing. > > In a sense the behavior in this patch is kind of an optimization of that > > approach, but I think implementing it directly would eliminate the need > > to mostly duplicate the put helper, perhaps making things a bit more > > natural. We could still fall into bch2_journal_reclaim_fast() from > > res_put(), but now only for cases where the reservation outlives the > > active journal buffer (due to a journal flush or whatever). That also > > still addresses the race by properly using the reference count. I'd > > probably have to try it to be sure I'm not missing something, but... > > thoughts? > > We can do your approach if that's what you feel is going to be cleanest, > but if we go that way here's my two main concerns: > Ok.. FWIW, it's not so much that I think it's the cleanest (I agree that the variant discussed above probably ends up cleaner code-wise), but rather that it provides sufficient confidence we won't just have to revisit the same underlying problem the next time some bit of code assumes that an outstanding reservation guarantees a usable pin list. > - we can't put more slowpath code into the inline fastpath, remember > this is all inlined into the main __bch2_trans_commit() path > Sure. I missed that closure_call() was itself inlined. I think that is what initially confused me wrt to the feedback on patch 2. I'll try to keep that helper external and perhaps just rename it. > - your approach (the optimized version) means there's a new state > transition where we do work, i.e. when journal_state_count() hits 0. > Indeed. > So we have to make sure that that refcount isn't hitting 0 multiple > times. The appropriate assertion already exists in > journal_res_get_fast() - for a refcount to hit 0 multiple times it > must leave 0 while in use, and we check for that. > > Let's just add a comment by that EBUG_ON(!journal_state_count(new, > new.idx)) line indicating why it's now important. > Sounds reasonable. I'll try to come up with something. Thanks. Brian
