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

[Brian Foster <bfoster@redhat.com>]

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 <kent.overstreet@linux.dev>
> ---
>  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
> 
>