Re: [PATCH 07/17] bcachefs: Make journal replay more efficient

[Brian Foster <bfoster@redhat.com>]

On Fri, Nov 10, 2023 at 11:31:44AM -0500, Kent Overstreet wrote:
> Journal replay now first attempts to replay keys in sorted order,
> similar to how the btree write buffer flush path works.
> 
> Any keys that can not be replayed due to journal deadlock are then left
> for later and replayed in journal order, unpinning journal entries as we
> go.
> 
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> ---

Just a question and a nit..

>  fs/bcachefs/errcode.h  |  1 -
>  fs/bcachefs/recovery.c | 84 ++++++++++++++++++++++++++++--------------
>  2 files changed, 56 insertions(+), 29 deletions(-)
> 
...
> diff --git a/fs/bcachefs/recovery.c b/fs/bcachefs/recovery.c
> index 6eafff2557a4..3524d1e0003e 100644
> --- a/fs/bcachefs/recovery.c
> +++ b/fs/bcachefs/recovery.c
...
> @@ -166,26 +159,61 @@ static int bch2_journal_replay(struct bch_fs *c)
>  			goto err;
>  	}
>  
> -	for (i = 0; i < keys->nr; i++) {
> -		k = keys_sorted[i];
> +	/*
> +	 * First, attempt to replay keys in sorted order. This is more
> +	 * efficient, but some might fail if that would cause a journal
> +	 * deadlock.
> +	 */

What does "sorted order" mean here vs. when we sort by journal seq
below?

> +	for (size_t i = 0; i < keys->nr; i++) {
> +		cond_resched();
> +
> +		struct journal_key *k = keys->d + i;
> +
> +		ret = commit_do(trans, NULL, NULL,
> +				BTREE_INSERT_NOFAIL|
> +				BTREE_INSERT_JOURNAL_RECLAIM|
> +				(!k->allocated ? BTREE_INSERT_JOURNAL_REPLAY : 0),
> +			     bch2_journal_replay_key(trans, k));
> +		BUG_ON(!ret && !k->overwritten);
> +		if (ret) {
> +			ret = darray_push(&keys_sorted, k);
> +			if (ret)
> +				goto err;
> +		}
> +	}
> +
> +	/*
> +	 * Now, replay any remaining keys in the order in which they appear in
> +	 * the journal, unpinning those journal entries as we go:
> +	 */
> +	sort(keys_sorted.data, keys_sorted.nr,
> +	     sizeof(keys_sorted.data[0]),
> +	     journal_sort_seq_cmp, NULL);
>  
> +	darray_for_each(keys_sorted, kp) {
>  		cond_resched();
>  
> +		struct journal_key *k = *kp;
> +
>  		replay_now_at(j, k->journal_seq);
>  
> -		ret = bch2_trans_do(c, NULL, NULL,
> -				    BTREE_INSERT_NOFAIL|
> -				    (!k->allocated
> -				     ? BTREE_INSERT_JOURNAL_REPLAY|BCH_WATERMARK_reclaim
> -				     : 0),
> +		ret = commit_do(trans, NULL, NULL,
> +				BTREE_INSERT_NOFAIL|
> +				(!k->allocated
> +				 ? BTREE_INSERT_JOURNAL_REPLAY|BCH_WATERMARK_reclaim
> +				 : 0),
>  			     bch2_journal_replay_key(trans, k));
> -		if (ret) {
> -			bch_err(c, "journal replay: error while replaying key at btree %s level %u: %s",
> -				bch2_btree_id_str(k->btree_id), k->level, bch2_err_str(ret));
> +		bch_err_msg(c, ret, "while replaying key at btree %s level %u:",
> +			    bch2_btree_id_str(k->btree_id), k->level);
> +		if (ret)
>  			goto err;
> -		}
> +
> +		BUG_ON(!k->overwritten);
>  	}
>  
> +	bch2_trans_put(trans);
> +	trans = NULL;
> +

Nit: This looks spurious w/ the exit path, just FWIW.

Brian

>  	replay_now_at(j, j->replay_journal_seq_end);
>  	j->replay_journal_seq = 0;
>  
> @@ -196,10 +224,10 @@ static int bch2_journal_replay(struct bch_fs *c)
>  	if (keys->nr && !ret)
>  		bch2_journal_log_msg(c, "journal replay finished");
>  err:
> -	kvfree(keys_sorted);
> -
> -	if (ret)
> -		bch_err_fn(c, ret);
> +	if (trans)
> +		bch2_trans_put(trans);
> +	darray_exit(&keys_sorted);
> +	bch_err_fn(c, ret);
>  	return ret;
>  }
>  
> -- 
> 2.42.0
> 
>