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

[Jan Kara]

On Mon 28-09-26 20:29:18, Qiliang Yuan wrote:
> The jbd2 checkpoint shrinker's count_objects() callback,
> jbd2_journal_shrink_count(), reports the number of checkpointed
> journal_head objects via the j_checkpoint_jh_count percpu counter.
> do_shrink_slab() reads this count exactly once per invocation and
> derives a one-shot scan budget (total_scan) from it, then repeatedly
> calls scan_objects() in fixed-size batches until that budget is
> exhausted.
> 
> jbd2_journal_shrink_scan() never updates sc->nr_scanned, even though
> include/linux/shrinker.h documents that "the callee should track its
> actual progress" in that field. Because sc->nr_scanned defaults to
> sc->nr_to_scan before every call, do_shrink_slab() always believes a
> full batch was examined, regardless of what
> jbd2_journal_shrink_checkpoint_list() actually did. That helper
> already tracks real progress through its own nr_to_scan in/out
> parameter: it decrements it once per transaction only by the number
> of buffers actually freed, and returns immediately without touching
> it at all when journal->j_checkpoint_transactions is NULL, or when
> every checkpointed buffer it looks at is still busy (still being
> written back) and gets skipped by JBD2_SHRINK_BUSY_SKIP. In both
> cases jbd2_journal_shrink_scan() currently reports zero progress to
> its caller (nr_shrunk == 0) while leaving sc->nr_scanned at its
> default nonzero value, so do_shrink_slab() has no way to tell "there
> was nothing to do this pass" from "a full batch was scanned and none
> of it was freeable", and keeps calling scan_objects() until the
> original, one-shot total_scan budget computed from the percpu count
> is drained, even though no further calls in this reclaim pass can
> make progress.
> 
> Compute sc->nr_scanned in jbd2_journal_shrink_scan() from the
> difference between the nr_to_scan value passed in and the value left
> behind by jbd2_journal_shrink_checkpoint_list(), and return
> SHRINK_STOP once that difference comes back as zero, since that only
> happens when there is genuinely nothing this pass can reclaim.
> 
> 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", while
> tracing the jbd2_shrink_* tracepoints:
> 
>                       total scan_objects()   calls with
>                       calls                  nr_shrunk == 0
>   before this patch   168                    168 (100%)
>   after this patch      6                      6 (100%)
> 
> In both runs every call found the checkpointed journal_head objects
> still busy, so nr_shrunk stayed at 0 throughout (this is a different
> trigger than the empty-list case in the analogous ext4 extents_status
> shrinker fix, but the same missing sc->nr_scanned accounting). Before
> this patch, do_shrink_slab() kept retrying at the same priority level
> until its one-shot budget ran out; after this patch SHRINK_STOP makes
> it give up after a single failed attempt per priority level instead.
> 
> Signed-off-by: Qiliang Yuan <odys.yuan@xxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/jbd2/journal.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 09efa337649e2..2602e5946cc14 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1262,10 +1262,26 @@ 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);
>  
> +	/*
> +	 * j_checkpoint_jh_count is a percpu counter, so count_objects() can
> +	 * report a stale/approximate value that is still positive even
> +	 * though the checkpoint list is actually empty by the time we get
> +	 * here. When that happens jbd2_journal_shrink_checkpoint_list()
> +	 * returns immediately having examined nothing (sc->nr_scanned == 0).
> +	 * Without SHRINK_STOP, do_shrink_slab() has no way to tell "nothing
> +	 * was there" from "a full batch was scanned and none of it was
> +	 * freeable" and will keep calling us with the same stale freeable
> +	 * count until its scan budget for this priority level is exhausted
> +	 * one batch at a time.
> +	 */
> +	if (sc->nr_scanned == 0)
> +		return SHRINK_STOP;
> +
>  	return nr_shrunk;
>  }
>  
> 
> ---
> base-commit: 502d801f0ab03e4f32f9a33d203154ce84887921
> change-id: 20260928-fix-jbd2-shrink-scan-nr-scanned-c3098cd901e2
> 
> Best regards,
> -- 
> Qiliang Yuan <odys.yuan@xxxxxxxxx>
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR