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

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

[Qiliang Yuan]

jbd2_journal_shrink_scan() never updates sc->nr_scanned, so
do_shrink_slab() always assumes a full batch was examined and keeps
calling scan_objects() until the one-shot budget derived from the
(possibly stale) percpu checkpoint count is drained, even when the
checkpoint list has nothing left to offer.

journal_shrink_one_cp_list() also conflates buffers examined with
buffers freed: it walks every journal_head in a transaction's list,
including ones skipped as busy, but only counts the ones it frees.

Add an nr_scanned output parameter to journal_shrink_one_cp_list()
that counts every journal_head examined, and use it to decrement the
scan budget in jbd2_journal_shrink_checkpoint_list() instead of the
freed count. Derive sc->nr_scanned from that examined count in
jbd2_journal_shrink_scan().

Trigger SHRINK_STOP on nr_shrunk == 0 (nothing freed) rather than
sc->nr_scanned == 0 (nothing examined): a checkpoint list full of
busy buffers can honestly report a full batch scanned while freeing
nothing, and retrying immediately will not speed up in-flight
writeback.

Tested by creating 20000 small files without an explicit sync and
dropping caches, tracing jbd2_shrink_*:

              scan_objects() calls   calls with nr_shrunk == 0
  before             168                    168 (100%)
  after                7                      6 (86%)

Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
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.

v1: https://lore.kernel.org/r/20260928-fix-jbd2-shrink-scan-nr-scanned-v1-1-e6f4016ec699@gmail.com
---
 fs/jbd2/checkpoint.c | 23 +++++++++++++++++------
 fs/jbd2/journal.c    | 17 +++++++++++++++++
 2 files changed, 34 insertions(+), 6 deletions(-)

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
@@ -361,27 +361,35 @@ int jbd2_cleanup_journal_tail(journal_t *journal)
  * Find all the written-back checkpoint buffers in the given list
  * and try to release them. If the whole transaction is released, set
  * the 'released' parameter. Return the number of released checkpointed
- * buffers.
+ * buffers. Store the number of buffers this function actually looked
+ * at, whether or not they were released, in '*nr_scanned' (a busy
+ * buffer skipped via JBD2_SHRINK_BUSY_SKIP still counts here, since
+ * walking it is real work, unlike the freed count).
  *
  * Called with j_list_lock held.
  */
 static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
 						enum jbd2_shrink_type type,
+						unsigned long *nr_scanned,
 						bool *released)
 {
 	struct journal_head *last_jh;
 	struct journal_head *next_jh = jh;
 	unsigned long nr_freed = 0;
+	unsigned long nr_examined = 0;
 	int ret;
 
 	*released = false;
-	if (!jh)
+	if (!jh) {
+		*nr_scanned = 0;
 		return 0;
+	}
 
 	last_jh = jh->b_cpprev;
 	do {
 		jh = next_jh;
 		next_jh = jh->b_cpnext;
+		nr_examined++;
 
 		if (type == JBD2_SHRINK_DESTROY) {
 			ret = __jbd2_journal_remove_checkpoint(jh);
@@ -404,6 +412,7 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
 			break;
 	} while (jh != last_jh);
 
+	*nr_scanned = nr_examined;
 	return nr_freed;
 }
 
@@ -424,7 +433,7 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
 	tid_t first_tid = 0, last_tid = 0, next_tid = 0;
 	tid_t tid = 0;
 	unsigned long nr_freed = 0;
-	unsigned long freed;
+	unsigned long freed, scanned;
 	bool first_set = false;
 
 again:
@@ -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);
 		if (*nr_to_scan == 0)
 			break;
 		if (need_resched() || spin_needbreak(&journal->j_list_lock))
@@ -502,6 +512,7 @@ void __jbd2_journal_clean_checkpoint_list(journal_t *journal,
 					  enum jbd2_shrink_type type)
 {
 	transaction_t *transaction, *last_transaction, *next_transaction;
+	unsigned long __always_unused scanned;
 	bool released;
 
 	WARN_ON_ONCE(type == JBD2_SHRINK_BUSY_SKIP);
@@ -516,7 +527,7 @@ void __jbd2_journal_clean_checkpoint_list(journal_t *journal,
 		transaction = next_transaction;
 		next_transaction = transaction->t_cpnext;
 		journal_shrink_one_cp_list(transaction->t_checkpoint_list,
-					   type, &released);
+					   type, &scanned, &released);
 		/*
 		 * This function only frees up some memory if possible so we
 		 * dont have an obligation to finish processing. Bail out if
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;
+
 	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 v2] 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: 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