[PATCH v3 0/2] ext4: fix shrinker scan budget accounting, plus a related cleanup

[PATCH v3 0/2] ext4: fix shrinker scan budget accounting, plus a related cleanup

[Qiliang Yuan]

This revises the ext4_es_scan() shrinker scan-budget fix per review,
and adds a small follow-up cleanup noticed during that review.

Patch 1 simplifies the fix to just returning SHRINK_STOP when
__es_shrink() frees nothing, dropping the sc->nr_scanned computation
entirely (Zhang Yi and Sashiko AI both pointed out it added risk
without a corresponding benefit: do_shrink_slab() discards
sc->nr_scanned on SHRINK_STOP anyway, and computing an accurate but
small nonzero value could reduce the SHRINK_STOP hit rate under light
background load).

Patch 2 removes __es_shrink()'s locked_ei parameter, which Zhang Yi
noted is dead: its sole caller always passes NULL.

Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
V2 -> V3:
- Drop the sc->nr_scanned computation entirely; just return
  SHRINK_STOP when __es_shrink() frees nothing (Zhang Yi, Sashiko AI
  review).
- Add patch 2/2: remove the now-dead locked_ei parameter (Zhang Yi).
- Add a Fixes: tag for the commit that split count_objects()/
  scan_objects() apart, and Cc: stable, matching the equivalent jbd2
  fix.
- Re-measure test data against the simplified fix (429 -> 238 calls).

V1 -> V2:
- Correct the comment and commit message: nr_scanned == 0 is not
  only reachable when sbi->s_es_list is genuinely empty, also when
  every inode walked was skipped or had nothing currently
  shrinkable. No code or test data changes.

v2: https://lore.kernel.org/r/20260929-fix-ext4-es-scan-nr-scanned-v2-1-f4e8f6f6b8b1@gmail.com
v1: https://lore.kernel.org/r/20260928-fix-ext4-es-scan-nr-scanned-v1-1-91d88228b0c8@gmail.com

---
Qiliang Yuan (2):
      ext4: fix shrinker scan budget accounting in ext4_es_scan()
      ext4: remove unused locked_ei parameter from __es_shrink()

 fs/ext4/extents_status.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)
---
base-commit: 502d801f0ab03e4f32f9a33d203154ce84887921
change-id: 20260928-fix-ext4-es-scan-nr-scanned-5af70744f6cd

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

[PATCH v3 1/2] ext4: fix shrinker scan budget accounting in ext4_es_scan()

[Qiliang Yuan]

The extents_status shrinker's count_objects() callback,
ext4_es_count(), reports the number of shrinkable extent_status
objects via a 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.

ext4_es_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 is
drained, even when sbi->s_es_list has had nothing left to give for a
while.

Return SHRINK_STOP whenever __es_shrink() 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 fallocate(2)-ing 10000 4K files (to populate the shrinker
with reclaimable unwritten extents without also exercising the
extent_status "referenced" second-chance path, which needs a
separate two-pass accounting of its own) and triggering
"echo 2 > /proc/sys/vm/drop_caches", while tracing the
ext4_es_shrink* tracepoints:

                      total scan_objects()   calls with
                      calls                  nr_shrunk == 0
  before this patch   429                    189 (44%)
  after this patch    238                      1 (0.4%)

Fixes: 1ab6c4997e04 ("fs: convert fs shrinkers to new scan/count API")
Cc: stable@vger.kernel.org
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
 fs/ext4/extents_status.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 6e4a191e82191..d88af807d36e7 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -1783,6 +1783,16 @@ static unsigned long ext4_es_scan(struct shrinker *shrink,
 
 	ret = percpu_counter_read_positive(&sbi->s_es_stats.es_stats_shk_cnt);
 	trace_ext4_es_shrink_scan_exit(sbi->s_sb, nr_shrunk, ret);
+
+	/*
+	 * es_stats_shk_cnt is a percpu counter, so it can stay positive
+	 * after sbi->s_es_list has nothing left to give. Stop this reclaim
+	 * pass once a call frees nothing, instead of burning through
+	 * do_shrink_slab()'s scan budget against a stale count.
+	 */
+	if (nr_shrunk == 0)
+		return SHRINK_STOP;
+
 	return nr_shrunk;
 }
 

-- 
2.43.0

Re: [PATCH v3 1/2] ext4: fix shrinker scan budget accounting in ext4_es_scan()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ext4: premature SHRINK_STOP in ext4_es_scan() breaks LRU reclaim
--

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

ext4: fix shrinker scan budget accounting in ext4_es_scan()

The extents_status shrinker's count_objects() callback reports the number of
shrinkable extent_status objects via a percpu counter. This patch returns
SHRINK_STOP when __es_shrink() frees nothing to prevent burning through the
scan budget against a stale freeable count.

> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 6e4a191e82191..d88af807d36e7 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -1783,6 +1783,16 @@ static unsigned long ext4_es_scan(struct shrinker *shrink,
>  
>  	ret = percpu_counter_read_positive(&sbi->s_es_stats.es_stats_shk_cnt);
>  	trace_ext4_es_shrink_scan_exit(sbi->s_sb, nr_shrunk, ret);
> +
> +	/*
> +	 * es_stats_shk_cnt is a percpu counter, so it can stay positive
> +	 * after sbi->s_es_list has nothing left to give. Stop this reclaim
> +	 * pass once a call frees nothing, instead of burning through
> +	 * do_shrink_slab()'s scan budget against a stale count.
> +	 */
> +	if (nr_shrunk == 0)
> +		return SHRINK_STOP;

[Severity: High]
Does this prematurely abort the shrinker when scanning referenced items?

If __es_shrink() processes a full batch (e.g., 128 items) but all of them are
referenced, it clears their referenced bit (aging them) but frees none,
resulting in nr_shrunk == 0.

For example, in es_do_reclaim_extents():

	(*nr_to_scan)--;
	node = rb_next(&es->rb_node);

	if (ext4_es_must_keep(es))
		goto next;
	if (ext4_es_is_referenced(es)) {
		ext4_es_clear_referenced(es);
		goto next;
	}

Because scanning referenced or unfreeable extents correctly consumes the scan
budget without incrementing nr_shrunk, won't this conflate "scanned a batch of
hot items" with "the cache is empty", breaking the second-chance LRU algorithm
under memory pressure?

>  	return nr_shrunk;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-fix-ext4-es-scan-nr-scanned-v3-0-9aef0be87d21@gmail.com?part=1

Re: [PATCH v3 1/2] ext4: fix shrinker scan budget accounting in ext4_es_scan()

[Jan Kara]

On Wed 30-09-26 15:11:37, Qiliang Yuan wrote:
> The extents_status shrinker's count_objects() callback,
> ext4_es_count(), reports the number of shrinkable extent_status
> objects via a 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.
> 
> ext4_es_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 is
> drained, even when sbi->s_es_list has had nothing left to give for a
> while.
> 
> Return SHRINK_STOP whenever __es_shrink() 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 fallocate(2)-ing 10000 4K files (to populate the shrinker
> with reclaimable unwritten extents without also exercising the
> extent_status "referenced" second-chance path, which needs a
> separate two-pass accounting of its own) and triggering
> "echo 2 > /proc/sys/vm/drop_caches", while tracing the
> ext4_es_shrink* tracepoints:
> 
>                       total scan_objects()   calls with
>                       calls                  nr_shrunk == 0
>   before this patch   429                    189 (44%)
>   after this patch    238                      1 (0.4%)
> 
> Fixes: 1ab6c4997e04 ("fs: convert fs shrinkers to new scan/count API")
> Cc: stable@vger.kernel.org
> Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>

...

> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 6e4a191e82191..d88af807d36e7 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -1783,6 +1783,16 @@ static unsigned long ext4_es_scan(struct shrinker *shrink,
>  
>  	ret = percpu_counter_read_positive(&sbi->s_es_stats.es_stats_shk_cnt);
>  	trace_ext4_es_shrink_scan_exit(sbi->s_sb, nr_shrunk, ret);
> +
> +	/*
> +	 * es_stats_shk_cnt is a percpu counter, so it can stay positive
> +	 * after sbi->s_es_list has nothing left to give. Stop this reclaim
> +	 * pass once a call frees nothing, instead of burning through
> +	 * do_shrink_slab()'s scan budget against a stale count.
> +	 */
> +	if (nr_shrunk == 0)
> +		return SHRINK_STOP;
> +

I think this is wrong. It breaks proper aging of es entries. It can easily
happen that nr_shrunk is 0 because we've just cleared EXTENT_STATUS_REFERENCED
flag on each es we've met. I think your previous version was actually
better as much as I agree with Yi that there are some chances for increased
scanning as well.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 2/2] ext4: remove unused locked_ei parameter from __es_shrink()

[Qiliang Yuan]

__es_shrink()'s locked_ei parameter exists to let a caller pass in an
inode it already holds i_es_lock on, so the shrinker walk can skip
that inode instead of deadlocking on write_trylock(), and fall back
to reclaiming from it directly if nothing else could be freed. Its
only caller, ext4_es_scan(), always passes NULL.

Drop the parameter along with the two code paths that exist solely to
support it: the ei == locked_ei skip check, and the trailing
es_reclaim_extents(locked_ei, ...) fallback, neither of which can ever
run while locked_ei is always NULL.

Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
 fs/ext4/extents_status.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index d88af807d36e7..efbccb29d5fd1 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -183,8 +183,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 			      int *reserved, struct extent_status *res,
 			      struct extent_status *prealloc);
 static int es_reclaim_extents(struct ext4_inode_info *ei, int *nr_to_scan);
-static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
-		       struct ext4_inode_info *locked_ei);
+static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan);
 static int __revise_pending(struct inode *inode, ext4_lblk_t lblk,
 			    ext4_lblk_t len,
 			    struct pending_reservation **prealloc);
@@ -1669,8 +1668,7 @@ void ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 	ext4_da_release_space(inode, reserved);
 }
 
-static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
-		       struct ext4_inode_info *locked_ei)
+static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan)
 {
 	struct ext4_inode_info *ei;
 	struct ext4_es_stats *es_stats;
@@ -1706,7 +1704,7 @@ static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
 			continue;
 		}
 
-		if (ei == locked_ei || !write_trylock(&ei->i_es_lock)) {
+		if (!write_trylock(&ei->i_es_lock)) {
 			nr_skipped++;
 			continue;
 		}
@@ -1734,9 +1732,6 @@ static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
 		goto retry;
 	}
 
-	if (locked_ei && nr_shrunk == 0)
-		nr_shrunk = es_reclaim_extents(locked_ei, &nr_to_scan);
-
 out:
 	scan_time = ktime_to_ns(ktime_sub(ktime_get(), start_time));
 	if (likely(es_stats->es_stats_scan_time))
@@ -1779,7 +1774,7 @@ static unsigned long ext4_es_scan(struct shrinker *shrink,
 	ret = percpu_counter_read_positive(&sbi->s_es_stats.es_stats_shk_cnt);
 	trace_ext4_es_shrink_scan_enter(sbi->s_sb, nr_to_scan, ret);
 
-	nr_shrunk = __es_shrink(sbi, nr_to_scan, NULL);
+	nr_shrunk = __es_shrink(sbi, nr_to_scan);
 
 	ret = percpu_counter_read_positive(&sbi->s_es_stats.es_stats_shk_cnt);
 	trace_ext4_es_shrink_scan_exit(sbi->s_sb, nr_shrunk, ret);

-- 
2.43.0

Re: [PATCH v3 2/2] ext4: remove unused locked_ei parameter from __es_shrink()

[sashiko-bot]

> __es_shrink()'s locked_ei parameter exists to let a caller pass in an
> inode it already holds i_es_lock on, so the shrinker walk can skip
> that inode instead of deadlocking on write_trylock(), and fall back
> to reclaiming from it directly if nothing else could be freed. Its
> only caller, ext4_es_scan(), always passes NULL.
> 
> Drop the parameter along with the two code paths that exist solely to
> support it: the ei == locked_ei skip check, and the trailing
> es_reclaim_extents(locked_ei, ...) fallback, neither of which can ever
> run while locked_ei is always NULL.
> 
> Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-fix-ext4-es-scan-nr-scanned-v3-0-9aef0be87d21@gmail.com?part=2

Re: [PATCH v3 2/2] ext4: remove unused locked_ei parameter from __es_shrink()

[Jan Kara]

On Wed 30-09-26 15:11:38, Qiliang Yuan wrote:
> __es_shrink()'s locked_ei parameter exists to let a caller pass in an
> inode it already holds i_es_lock on, so the shrinker walk can skip
> that inode instead of deadlocking on write_trylock(), and fall back
> to reclaiming from it directly if nothing else could be freed. Its
> only caller, ext4_es_scan(), always passes NULL.
> 
> Drop the parameter along with the two code paths that exist solely to
> support it: the ei == locked_ei skip check, and the trailing
> es_reclaim_extents(locked_ei, ...) fallback, neither of which can ever
> run while locked_ei is always NULL.
> 
> Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/ext4/extents_status.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index d88af807d36e7..efbccb29d5fd1 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -183,8 +183,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  			      int *reserved, struct extent_status *res,
>  			      struct extent_status *prealloc);
>  static int es_reclaim_extents(struct ext4_inode_info *ei, int *nr_to_scan);
> -static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
> -		       struct ext4_inode_info *locked_ei);
> +static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan);
>  static int __revise_pending(struct inode *inode, ext4_lblk_t lblk,
>  			    ext4_lblk_t len,
>  			    struct pending_reservation **prealloc);
> @@ -1669,8 +1668,7 @@ void ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  	ext4_da_release_space(inode, reserved);
>  }
>  
> -static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
> -		       struct ext4_inode_info *locked_ei)
> +static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan)
>  {
>  	struct ext4_inode_info *ei;
>  	struct ext4_es_stats *es_stats;
> @@ -1706,7 +1704,7 @@ static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
>  			continue;
>  		}
>  
> -		if (ei == locked_ei || !write_trylock(&ei->i_es_lock)) {
> +		if (!write_trylock(&ei->i_es_lock)) {
>  			nr_skipped++;
>  			continue;
>  		}
> @@ -1734,9 +1732,6 @@ static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
>  		goto retry;
>  	}
>  
> -	if (locked_ei && nr_shrunk == 0)
> -		nr_shrunk = es_reclaim_extents(locked_ei, &nr_to_scan);
> -
>  out:
>  	scan_time = ktime_to_ns(ktime_sub(ktime_get(), start_time));
>  	if (likely(es_stats->es_stats_scan_time))
> @@ -1779,7 +1774,7 @@ static unsigned long ext4_es_scan(struct shrinker *shrink,
>  	ret = percpu_counter_read_positive(&sbi->s_es_stats.es_stats_shk_cnt);
>  	trace_ext4_es_shrink_scan_enter(sbi->s_sb, nr_to_scan, ret);
>  
> -	nr_shrunk = __es_shrink(sbi, nr_to_scan, NULL);
> +	nr_shrunk = __es_shrink(sbi, nr_to_scan);
>  
>  	ret = percpu_counter_read_positive(&sbi->s_es_stats.es_stats_shk_cnt);
>  	trace_ext4_es_shrink_scan_exit(sbi->s_sb, nr_shrunk, ret);
> 
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR