Re: [PATCH v3] 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: SHRINK_STOP return bypasses nr_scanned accounting
--

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

    jbd2: fix shrinker scan budget accounting in jbd2_journal_shrink_scan()

This commit changes the jbd2 shrinker to report the actual number of
scanned objects via sc->nr_scanned. It also introduces a condition to
return SHRINK_STOP when no buffers were successfully freed, to prevent
do_shrink_slab() from looping over busy buffers.

> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 00f5a98f3d4fe..a61b7a59c0a0e 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1263,10 +1263,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: Medium]
Will returning SHRINK_STOP here bypass the intended accounting of
sc->nr_scanned?

Looking at do_shrink_slab() in mm/shrinker.c, when a shrinker returns
SHRINK_STOP, the loop breaks early:

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;
	...
}

Since the break happens before the accounting updates, won't this entirely
discard the scan progress reported by jbd2 when it hits a batch of busy
buffers? This appears to leave the scanned items unrecorded in vmstat and
leaves them un-subtracted from the deferred scan budget.

> +
>  	return nr_shrunk;
>  }
>

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