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

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

[Qiliang Yuan]

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@gmail.com>
---
 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@gmail.com>

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] jbd2: CPU spike due to incorrect shrinker nr_scanned accounting
--

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

jbd2: fix shrinker scan budget accounting in jbd2_journal_shrink_scan()

This commit updates the jbd2 checkpoint shrinker's scan budget accounting
by calculating the number of scanned items, and returning SHRINK_STOP if
no items were scanned.

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

[Severity: High]
Will this correctly account for the items that were scanned but not freed?

Looking at jbd2_journal_shrink_checkpoint_list() in fs/jbd2/checkpoint.c,
nr_to_scan is only decremented by the number of freed items:

    freed = journal_shrink_one_cp_list(transaction->t_checkpoint_list,
                                       JBD2_SHRINK_BUSY_SKIP, &released);
    nr_freed += freed;
    (*nr_to_scan) -= min(*nr_to_scan, freed);

Furthermore, journal_shrink_one_cp_list() explicitly skips busy buffers
without them counting towards the freed total:

    if (type == JBD2_SHRINK_BUSY_SKIP && jh->b_transaction)
        continue;

This effectively sets sc->nr_scanned to the number of freed items, not the
actual number of scanned items.

The shrinker API expects sc->nr_scanned to reflect the actual amount of work
done (items scanned). If the shrinker scans many busy buffers but frees very
few of them, it reports a tiny number of scanned items. 

Can this cause do_shrink_slab() to repeatedly re-invoke the shrinker without
meaningfully reducing its total_scan budget, leading to high CPU usage or
potential soft lockups under memory pressure?

>  
>  	count = percpu_counter_read_positive(&journal->j_checkpoint_jh_count);
>  	trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);

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

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@gmail.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								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@gmail.com>
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR