On Fri, Nov 10, 2023 at 11:31:40AM -0500, Kent Overstreet wrote:
> flush_fn is how we identify journal pins in debugfs - this is a
> debugging aid.
>
> Signed-off-by: Kent Overstreet <[email protected]>
> ---
> fs/bcachefs/btree_trans_commit.c | 9 ++++++++-
> fs/bcachefs/btree_update_interior.c | 21 ++++++++++++++++++---
> fs/bcachefs/btree_write_buffer.c | 18 +++++++-----------
> fs/bcachefs/journal_reclaim.c | 12 +++++++-----
> 4 files changed, 40 insertions(+), 20 deletions(-)
>
...
> diff --git a/fs/bcachefs/btree_write_buffer.c
> b/fs/bcachefs/btree_write_buffer.c
> index 5c398b05cb39..9e3107187e1d 100644
> --- a/fs/bcachefs/btree_write_buffer.c
> +++ b/fs/bcachefs/btree_write_buffer.c
...
> @@ -148,7 +151,8 @@ int __bch2_btree_write_buffer_flush(struct btree_trans
> *trans, unsigned commit_f
> if (!locked && !mutex_trylock(&wb->flush_lock))
> return 0;
>
> - bch2_journal_pin_copy(j, &pin, &wb->journal_pin, NULL);
> + bch2_journal_pin_copy(j, &pin, &wb->journal_pin,
> + bch2_btree_write_buffer_journal_flush);
> bch2_journal_pin_drop(j, &wb->journal_pin);
>
> s = btree_write_buffer_switch(wb);
> @@ -250,16 +254,8 @@ int __bch2_btree_write_buffer_flush(struct btree_trans
> *trans, unsigned commit_f
> if (!i->journal_seq)
> continue;
>
> - 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.
Brian
>
> ret = commit_do(trans, NULL, NULL,
> commit_flags|
> diff --git a/fs/bcachefs/journal_reclaim.c b/fs/bcachefs/journal_reclaim.c
> index 79a6fdc6e6ef..8fa05bedb7df 100644
> --- a/fs/bcachefs/journal_reclaim.c
> +++ b/fs/bcachefs/journal_reclaim.c
> @@ -378,14 +378,16 @@ static inline void bch2_journal_pin_set_locked(struct
> journal *j, u64 seq,
> {
> struct journal_entry_pin_list *pin_list = journal_seq_pin(j, seq);
>
> + /*
> + * flush_fn is how we identify journal pins in debugfs, so must always
> + * exist, even if it doesn't do anything:
> + */
> + BUG_ON(!flush_fn);
> +
> atomic_inc(&pin_list->count);
> pin->seq = seq;
> pin->flush = flush_fn;
> -
> - if (flush_fn)
> - list_add(&pin->list, &pin_list->list[type]);
> - else
> - list_add(&pin->list, &pin_list->flushed);
> + list_add(&pin->list, &pin_list->list[type]);
> }
>
> void bch2_journal_pin_copy(struct journal *j,
> --
> 2.42.0
>
>
