Re: [PATCH RESEND v5] jbd2: fix shrinker scan budget accounting in jbd2_journal_shrink_scan()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] jbd2: repeated busy buffer rescans in jbd2_journal_shrink_scan()
--

commit e6c15f2237536f947ab070ac978601d8d9849323
Author: Qiliang Yuan <odys.yuan@gmail.com>

jbd2: fix shrinker scan budget accounting in jbd2_journal_shrink_scan()

This commit updates sc->nr_scanned in jbd2_journal_shrink_scan() based on the
remaining nr_to_scan count from jbd2_journal_shrink_checkpoint_list() and
returns SHRINK_STOP when sc->nr_scanned is zero.

> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 00f5a98f3d4fe..eac5f84a1afc5 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1263,10 +1263,22 @@ static unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
>  	trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
>  
>  	nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
> +	sc->nr_scanned = sc->nr_to_scan - nr_to_scan;
>  
>  	count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
>  	trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
>  
> +	/*
> +	 * Key SHRINK_STOP off sc->nr_scanned (buffers actually examined),
> +	 * not nr_shrunk (buffers actually freed): a batch can find every
> +	 * buffer in its transactions busy and free none, while later
> +	 * transactions may still hold buffers whose writeback has
> +	 * completed. sc->nr_scanned only comes back 0 when the checkpoint
> +	 * list had nothing left to walk.
> +	 */
> +	if (sc->nr_scanned == 0)
> +		return SHRINK_STOP;

[Severity: Medium]
What happens here when the checkpoint list has already been walked to
last_transaction and only a small number of busy buffers (for example,
1 <= sc->nr_scanned < sc->nr_to_scan) remain on the list?

In fs/jbd2/checkpoint.c, jbd2_journal_shrink_checkpoint_list() loops via
goto again while *nr_to_scan is non-zero and journal->j_shrink_transaction is
non-NULL:

	if (transaction != last_transaction) {
		journal->j_shrink_transaction = next_transaction;
		next_tid = next_transaction->t_tid;
	} else {
		journal->j_shrink_transaction = NULL;
		next_tid = 0;
	}

	spin_unlock(&journal->j_list_lock);
	cond_resched();

	if (*nr_to_scan && journal->j_shrink_transaction)
		goto again;

Because of that loop, jbd2_journal_shrink_checkpoint_list() only returns with
nr_to_scan > 0 (meaning sc->nr_scanned < sc->nr_to_scan) when the end of the
checkpoint list (last_transaction) has been reached and
journal->j_shrink_transaction has been reset to NULL.

If earlier batches freed the clean buffers and, say, 1 busy buffer remains on
the checkpoint list, jbd2_journal_shrink_checkpoint_list() walks that 1 busy
buffer, reaches last_transaction, resets journal->j_shrink_transaction to
NULL, and returns with sc->nr_scanned == 1.

Since sc->nr_scanned == 1 is non-zero, jbd2_journal_shrink_scan() does not
return SHRINK_STOP. Back in mm/shrinker.c:do_shrink_slab():

		ret = shrinker->scan_objects(shrinker, shrinkctl);
		if (ret == SHRINK_STOP)
			break;
		freed += ret;

		count_vm_events(SLABS_SCANNED, shrinkctl->nr_scanned);
		total_scan -= shrinkctl->nr_scanned;
		scanned += shrinkctl->nr_scanned;

total_scan is decremented by only 1 instead of sc->nr_to_scan (128). On the
next iteration of do_shrink_slab(), jbd2_journal_shrink_checkpoint_list()
starts over from the head of journal->j_checkpoint_transactions and rescans
the exact same busy buffer, multiplying the number of scan_objects() calls
and journal->j_list_lock acquisitions on the remaining scan budget by up to
SHRINK_BATCH (128x).

In addition, when the checkpoint list is empty (sc->nr_scanned == 0) and
jbd2_journal_shrink_scan() returns SHRINK_STOP before total_scan is drained,
do_shrink_slab() breaks out with scanned < nr + delta and banks the un-scanned
phantom budget from the stale percpu counter into shrinker->nr_deferred:

	next_deferred = max_t(long, (nr + delta - scanned), 0);
	next_deferred = min(next_deferred, (2 * freeable));
	new_nr = add_nr_deferred(next_deferred, shrinker, shrinkctl);

Would it make more sense to check nr_to_scan != 0 (indicating
jbd2_journal_shrink_checkpoint_list() reached the end of the checkpoint list
before exhausting the batch budget) or to leave sc->nr_scanned untouched so
the stale budget is consumed without deferring phantom work or rescanning the
same busy buffers up to 128 times?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-fix-jbd2-shrink-scan-nr-scanned-v5-1-ebd1ecc59a96@gmail.com?part=1