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