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

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

[Qiliang Yuan]

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@vger.kernel.org
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
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@gmail.com
v2: https://lore.kernel.org/r/20260928-fix-jbd2-shrink-scan-nr-scanned-v2-1-7e1efec3afdf@gmail.com
v1: https://lore.kernel.org/r/20260928-fix-jbd2-shrink-scan-nr-scanned-v1-1-e6f4016ec699@gmail.com
---
 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;
+
 	return nr_shrunk;
 }
 

---
base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
change-id: 20260928-fix-jbd2-shrink-scan-nr-scanned-c3098cd901e2

Best regards,
-- 
Qiliang Yuan <odys.yuan@gmail.com>

Re: [PATCH v4] 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] fs/jbd2: premature reclaim abort in jbd2_journal_shrink_scan()
--

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

jbd2: fix shrinker scan budget accounting in jbd2_journal_shrink_scan()

This commit modifies jbd2_journal_shrink_scan() to return SHRINK_STOP
when jbd2_journal_shrink_checkpoint_list() frees zero items. The goal
is to prevent the shrinker from continually scanning a stale freeable count
when the checkpoint list only contains busy buffers.

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

[Severity: High]
Could this unintentionally abort the entire reclaim pass when the code hits
a chunk of busy buffers or gets interrupted by the scheduler?

If do_shrink_slab() provides a scan budget and the first batch of buffers
happen to be busy writeback buffers, jbd2_journal_shrink_checkpoint_list()
will return 0. This change then translates that 0 into SHRINK_STOP, which
aborts the rest of the scanning budget for this reclaim pass.

Additionally, jbd2_journal_shrink_checkpoint_list() can return 0 early due
to scheduling or lock contention:

fs/jbd2/checkpoint.c:jbd2_journal_shrink_checkpoint_list()
    ...
		if (need_resched() || spin_needbreak(&journal->j_list_lock))
			break;
    ...

Will returning SHRINK_STOP in these scenarios prematurely skip remaining
transactions that might contain clean, freeable buffers?

> +
>  	return nr_shrunk;
>  }
>

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

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@vger.kernel.org
> Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
> 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@gmail.com
> v2: https://lore.kernel.org/r/20260928-fix-jbd2-shrink-scan-nr-scanned-v2-1-7e1efec3afdf@gmail.com
> v1: https://lore.kernel.org/r/20260928-fix-jbd2-shrink-scan-nr-scanned-v1-1-e6f4016ec699@gmail.com
> ---
>  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@suse.com>
SUSE Labs, CR