Re: [PATCH v2] 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:
- [High] jbd2: shrinker scan count under-reporting and improper SHRINK_STOP
--

commit 210f96938647f489c8074cf3aa01e6d2cbdb8fe9
Author: Qiliang Yuan <odys.yuan@gmail.com>
jbd2: fix shrinker scan budget accounting in jbd2_journal_shrink_scan()

This commit modifies the jbd2 shrinker to count examined buffers rather than
just freed ones when decrementing the scan budget. It also triggers
SHRINK_STOP when no buffers are freed to avoid immediate retries.

> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index 1508e2f544621..3569e32500b7c 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c

[ ... ]

> @@ -458,9 +467,10 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
>  		tid = transaction->t_tid;
>  
>  		freed = journal_shrink_one_cp_list(transaction->t_checkpoint_list,
> -						   JBD2_SHRINK_BUSY_SKIP, &released);
> +						   JBD2_SHRINK_BUSY_SKIP, &scanned,
> +						   &released);
>  		nr_freed += freed;
> -		(*nr_to_scan) -= min(*nr_to_scan, freed);
> +		(*nr_to_scan) -= min(*nr_to_scan, scanned);

[Severity: High]
By capping the decrement with min(), does this artificially deflate the
reported scanned items? If a transaction is large, it seems this would hide
the true number of examined buffers from the caller.

>  		if (*nr_to_scan == 0)
>  			break;
>  		if (need_resched() || spin_needbreak(&journal->j_list_lock))

[ ... ]

> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 09efa337649e2..e35d488cbfd23 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c

[ ... ]

> @@ -1262,10 +1262,27 @@ 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);
>  
> +	/*
> +	 * Give up on this reclaim pass if this call didn't manage to free
> +	 * anything. This is deliberately based on nr_shrunk, not on
> +	 * sc->nr_scanned: a checkpoint list can be full of buffers that are
> +	 * still busy being written back, in which case a call can
> +	 * legitimately scan (and correctly report through sc->nr_scanned)
> +	 * a full batch of them without freeing a single one. Retrying
> +	 * immediately within the same synchronous reclaim pass is not going
> +	 * to let any of that in-flight writeback complete any sooner, so
> +	 * there is nothing to gain from letting do_shrink_slab() keep
> +	 * calling us against the same stale freeable count until its
> +	 * one-shot scan budget for this priority level is exhausted.
> +	 */
> +	if (nr_shrunk == 0)
> +		return SHRINK_STOP;

[Severity: High]
Does returning SHRINK_STOP when nr_shrunk == 0 cause do_shrink_slab() to
break early and skip accounting for sc->nr_scanned?

It appears the scanned items are not subtracted from total_scan nor added to
scanned, so they are dumped into nr_deferred. Could this inflate the
deferred work budget, tricking the core shrinker into applying more pressure
in subsequent passes?

Additionally, does aborting early here prevent the shrinker from making
progress on subsequent transactions that might actually contain freeable
items?

> +
>  	return nr_shrunk;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-fix-jbd2-shrink-scan-nr-scanned-v2-1-7e1efec3afdf@gmail.com?part=1