Re: [PATCH 03/17] bcachefs: Journal pins must always have a flush_fn

[Brian Foster <bfoster@redhat.com>]

On Mon, Nov 13, 2023 at 11:36:23AM -0500, Kent Overstreet wrote:
> On Mon, Nov 13, 2023 at 10:22:13AM -0500, Brian Foster wrote:
> > On Fri, Nov 10, 2023 at 11:31:40AM -0500, Kent Overstreet wrote:
> > > -		if (i->journal_seq > pin.seq) {
> > > -			struct journal_entry_pin pin2;
> > > -
> > > -			memset(&pin2, 0, sizeof(pin2));
> > > -
> > > -			bch2_journal_pin_add(j, i->journal_seq, &pin2, NULL);
> > > -			bch2_journal_pin_drop(j, &pin);
> > > -			bch2_journal_pin_copy(j, &pin, &pin2, NULL);
> > > -			bch2_journal_pin_drop(j, &pin2);
> > > -		}
> > > +		bch2_journal_pin_update(j, i->journal_seq, &pin,
> > > +			      bch2_btree_write_buffer_journal_flush);
> > 
> > Hmm.. I recall looking at this on the previous improvements to this
> > path, but I don't quite remember why I didn't make this sort of change.
> > The existing code implies a race (i.e., using pin2 to ensure the pin is
> > never fully absent from the pin list) as opposed to what _pin_update()
> > does (remove then add). Any idea why the existing code does what it does
> > and/or can you explain why this change is safe? Thanks.
> 
> Perhaps you missed it because journal_pin_update() could be named
> better? journal_pin_add() and journal_pin_update() have opposite
> behaviour when asked to overrwite an existing pin, I would like names
> that make that more explicit.
> 

Quite possible..

> But I don't see any races possible here: journal_pin_update() first
> checks pin->seq, and that's entirely under the control of this thread so
> we're fine, and if it overwrites an existing pin the drop and the set
> are done together under the journal lock.
> 

Yeah, I don't mean to imply there actually is a race. I just mean that
it looks like the old code took care to add the new pin before removing
the old for some reason, and I'm just curious what that reason is. ;)

Brian