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

[Jan Kara]

On Wed 30-09-26 15:11:58, Qiliang Yuan wrote:
> jbd2_journal_shrink_scan() never signals SHRINK_STOP, so
> do_shrink_slab() has no way to tell "there was nothing to reclaim
> this pass" from "a full batch was scanned and none of it was
> freeable". It keeps calling scan_objects() until the original,
> one-shot total_scan budget derived from the (possibly stale) percpu
> checkpoint count is drained, even when every buffer in the
> checkpoint list is busy and nothing gets freed.
> 
> Return SHRINK_STOP whenever jbd2_journal_shrink_checkpoint_list()
> frees nothing, so do_shrink_slab() stops calling us for the rest of
> this reclaim pass instead of burning through its scan budget against
> the same stale freeable count.
> 
> Tested by creating 20000 small files without an explicit sync (to let
> jbd2's normal 5-second commit timer move them onto the checkpoint list
> while their buffers are still busy being written back) and then
> triggering "echo 2 > /proc/sys/vm/drop_caches", tracing jbd2_shrink_*:
> 
>               scan_objects() calls   calls with nr_shrunk == 0
>   before             176                    176 (100%)
>   after                8                      8 (100%)
> 
> Fixes: 4ba3fcdde7e3 ("jbd2,ext4: add a shrinker to release checkpointed buffers")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Qiliang Yuan <odys.yuan@xxxxxxxxx>
> Reviewed-by: Jan Kara <jack@xxxxxxx>
> ---
> V3 -> V4:
> - Drop the sc->nr_scanned computation entirely; just return
>   SHRINK_STOP when nr_shrunk is 0 (Zhang Yi, Sashiko AI review).
> - Add Jan Kara's Reviewed-by (carried over: the code is functionally
>   identical to what he reviewed in v1).
> 
> V2 -> V3:
> - Rebase onto v7.3-rc5. It already includes Max Kellermann's
>   "jbd2: bound shrinker scans by examined checkpoint buffers"
>   (15cb16496446), which independently fixes the busy-buffer
>   accounting in journal_shrink_one_cp_list()/
>   jbd2_journal_shrink_checkpoint_list() that v2 also touched. Drop
>   the now-redundant checkpoint.c changes; this revision only touches
>   journal.c, reusing the accurate nr_to_scan tracking Kellermann's
>   fix already provides.
> - Add a Fixes: tag for the commit that introduced
>   jbd2_journal_shrink_scan() without ever setting sc->nr_scanned.
> - Re-measure test data against the new baseline (176 -> 8 calls,
>   vs the old baseline's 168 -> 7).
> 
> V1 -> V2:
> - Count examined buffers, not just freed ones, in
>   journal_shrink_one_cp_list() (Sashiko AI review finding).
> - Trigger SHRINK_STOP on nr_shrunk == 0 instead of
>   sc->nr_scanned == 0.
> 
> v3: https://lore.kernel.org/r/20260928-fix-jbd2-shrink-scan-nr-scanned-v3-1-916caaa53420@xxxxxxxxx
> v2: https://lore.kernel.org/r/20260928-fix-jbd2-shrink-scan-nr-scanned-v2-1-7e1efec3afdf@xxxxxxxxx
> v1: https://lore.kernel.org/r/20260928-fix-jbd2-shrink-scan-nr-scanned-v1-1-e6f4016ec699@xxxxxxxxx
> ---
>  fs/jbd2/journal.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 00f5a98f3d4fe..c921afce7d061 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1267,6 +1267,16 @@ static unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
>  	count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
>  	trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
>  
> +	/*
> +	 * A checkpoint list full of busy buffers can keep reporting a
> +	 * stale, positive freeable count after nothing more can be
> +	 * reclaimed. Stop this reclaim pass once a call frees nothing,
> +	 * instead of burning through do_shrink_slab()'s scan budget
> +	 * retrying against buffers whose writeback won't finish any sooner.
> +	 */
> +	if (nr_shrunk == 0)
> +		return SHRINK_STOP;
> +

Again, this is wrong. The fact that we had to go through nr_to_scan bhs
which we couldn't reclaim doesn't mean that the next batch (which continues
where the current one ended) won't be able to find freeable bhs in the
following transactions.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR