[PATCH v2] ext4: fix shrinker scan budget accounting in ext4_es_scan()

[PATCH v2] 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 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 __es_shrink()
actually did. When __es_shrink() makes no progress and returns having
examined nothing, do_shrink_slab() has no way to tell "there was
nothing to scan" 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 though nothing in this
reclaim pass can make further progress.

Make __es_shrink() report the number of extent_status objects it
actually examined through a new nr_scanned output parameter, derived
from the existing per-extent nr_to_scan counter that es_reclaim_extents()
already decrements as it walks the tree. Have ext4_es_scan() copy this
value into sc->nr_scanned, and return SHRINK_STOP once nr_scanned comes
back as zero.

nr_scanned can come back as zero for more than one reason:
sbi->s_es_list can be genuinely empty, every inode walked this call
can have been momentarily skipped (precached, or lock contended), or
every inode walked can have had nothing currently shrinkable
(es_reclaim_extents() returns immediately without touching nr_to_scan
whenever an inode's shrinkable extent count is zero, independent of
locking). Treat all of these the same rather than only returning
SHRINK_STOP for a genuinely empty list: retrying instead, on the
assumption that the other cases are transient, leaves
do_shrink_slab()'s own scan budget undecremented whenever nr_scanned
stays zero, so it never terminates.

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    165                    1 (0.6%)
  after (rerun)        242                    1 (0.4%)

nr_skipped stayed at 0 throughout every run, confirming the wasted
calls came from the stale one-shot budget racing ahead of the real
list state, not from the existing precached/trylock skip paths.

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

v1: https://lore.kernel.org/r/20260928-fix-ext4-es-scan-nr-scanned-v1-1-91d88228b0c8@gmail.com
---
 fs/ext4/extents_status.c | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 6e4a191e82191..95e0f2d453257 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -184,7 +184,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 			      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);
+		       struct ext4_inode_info *locked_ei, int *nr_scanned);
 static int __revise_pending(struct inode *inode, ext4_lblk_t lblk,
 			    ext4_lblk_t len,
 			    struct pending_reservation **prealloc);
@@ -1670,7 +1670,7 @@ void ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 }
 
 static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
-		       struct ext4_inode_info *locked_ei)
+		       struct ext4_inode_info *locked_ei, int *nr_scanned)
 {
 	struct ext4_inode_info *ei;
 	struct ext4_es_stats *es_stats;
@@ -1679,6 +1679,7 @@ static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
 	int nr_to_walk;
 	int nr_shrunk = 0;
 	int retried = 0, nr_skipped = 0;
+	int orig_nr_to_scan = nr_to_scan;
 
 	es_stats = &sbi->s_es_stats;
 	start_time = ktime_get();
@@ -1738,6 +1739,8 @@ static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
 		nr_shrunk = es_reclaim_extents(locked_ei, &nr_to_scan);
 
 out:
+	*nr_scanned = orig_nr_to_scan - nr_to_scan;
+
 	scan_time = ktime_to_ns(ktime_sub(ktime_get(), start_time));
 	if (likely(es_stats->es_stats_scan_time))
 		es_stats->es_stats_scan_time = (scan_time +
@@ -1774,15 +1777,41 @@ static unsigned long ext4_es_scan(struct shrinker *shrink,
 {
 	struct ext4_sb_info *sbi = shrink->private_data;
 	int nr_to_scan = sc->nr_to_scan;
-	int ret, nr_shrunk;
+	int ret, nr_shrunk, nr_scanned;
 
 	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, NULL, &nr_scanned);
+	sc->nr_scanned = nr_scanned;
 
 	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);
+
+	/*
+	 * nr_scanned == 0 does not necessarily mean sbi->s_es_list is empty:
+	 * es_stats_shk_cnt is a percpu counter and can report a stale/
+	 * approximate value that is still positive after the list has
+	 * actually drained, but nr_scanned also stays 0 when every inode
+	 * __es_shrink() walked this call was momentarily skipped (precached,
+	 * or lock contended) or had nothing currently shrinkable
+	 * (es_reclaim_extents() returns without touching nr_to_scan whenever
+	 * ei->i_es_shk_nr is 0, which can persist for a given inode
+	 * independent of locking).
+	 *
+	 * Treat all of these the same and return SHRINK_STOP rather than
+	 * only doing so for a genuinely empty list: retrying instead, on
+	 * the assumption that the other cases are transient, does not
+	 * decrement do_shrink_slab()'s own scan budget when nr_scanned
+	 * stays 0, so it never terminates. Without SHRINK_STOP here,
+	 * do_shrink_slab() also has no way to tell "nothing was there" from
+	 * "nothing was scanned yet" 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 (nr_scanned == 0)
+		return SHRINK_STOP;
+
 	return nr_shrunk;
 }
 

---
base-commit: 502d801f0ab03e4f32f9a33d203154ce84887921
change-id: 20260928-fix-ext4-es-scan-nr-scanned-5af70744f6cd

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

Re: [PATCH v2] ext4: fix shrinker scan budget accounting in ext4_es_scan()

[Zhang Yi]

On 9/29/2026 10:20 AM, 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 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 __es_shrink()
> actually did. When __es_shrink() makes no progress and returns having
> examined nothing, do_shrink_slab() has no way to tell "there was
> nothing to scan" 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 though nothing in this
> reclaim pass can make further progress.
> 
> Make __es_shrink() report the number of extent_status objects it
> actually examined through a new nr_scanned output parameter, derived
> from the existing per-extent nr_to_scan counter that es_reclaim_extents()
> already decrements as it walks the tree. Have ext4_es_scan() copy this
> value into sc->nr_scanned, and return SHRINK_STOP once nr_scanned comes
> back as zero.
> 
> nr_scanned can come back as zero for more than one reason:
> sbi->s_es_list can be genuinely empty, every inode walked this call
> can have been momentarily skipped (precached, or lock contended), or
> every inode walked can have had nothing currently shrinkable
> (es_reclaim_extents() returns immediately without touching nr_to_scan
> whenever an inode's shrinkable extent count is zero, independent of
> locking). Treat all of these the same rather than only returning
> SHRINK_STOP for a genuinely empty list: retrying instead, on the
> assumption that the other cases are transient, leaves
> do_shrink_slab()'s own scan budget undecremented whenever nr_scanned
> stays zero, so it never terminates.
> 
> 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    165                    1 (0.6%)
>   after (rerun)        242                    1 (0.4%)
> 
> nr_skipped stayed at 0 throughout every run, confirming the wasted
> calls came from the stale one-shot budget racing ahead of the real
> list state, not from the existing precached/trylock skip paths.
> 
> Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>

Hi, Qiliang!

Thank you for the patch, this overall makes sense to me, just some
suggestions below.

> ---
> 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.
> 
> v1: https://lore.kernel.org/r/20260928-fix-ext4-es-scan-nr-scanned-v1-1-91d88228b0c8@gmail.com
> ---
>  fs/ext4/extents_status.c | 37 +++++++++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 6e4a191e82191..95e0f2d453257 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -184,7 +184,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  			      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);
> +		       struct ext4_inode_info *locked_ei, int *nr_scanned);


It looks like the locked_ei parameter is no longer used. If you are
available, could you add a patch to remove it as well?


>  static int __revise_pending(struct inode *inode, ext4_lblk_t lblk,
>  			    ext4_lblk_t len,
>  			    struct pending_reservation **prealloc);
> @@ -1670,7 +1670,7 @@ void ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  }
>  
>  static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
> -		       struct ext4_inode_info *locked_ei)
> +		       struct ext4_inode_info *locked_ei, int *nr_scanned)
>  {
>  	struct ext4_inode_info *ei;
>  	struct ext4_es_stats *es_stats;
> @@ -1679,6 +1679,7 @@ static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
>  	int nr_to_walk;
>  	int nr_shrunk = 0;
>  	int retried = 0, nr_skipped = 0;
> +	int orig_nr_to_scan = nr_to_scan;
>  
>  	es_stats = &sbi->s_es_stats;
>  	start_time = ktime_get();
> @@ -1738,6 +1739,8 @@ static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
>  		nr_shrunk = es_reclaim_extents(locked_ei, &nr_to_scan);
>  
>  out:
> +	*nr_scanned = orig_nr_to_scan - nr_to_scan;
> +
>  	scan_time = ktime_to_ns(ktime_sub(ktime_get(), start_time));
>  	if (likely(es_stats->es_stats_scan_time))
>  		es_stats->es_stats_scan_time = (scan_time +
> @@ -1774,15 +1777,41 @@ static unsigned long ext4_es_scan(struct shrinker *shrink,
>  {
>  	struct ext4_sb_info *sbi = shrink->private_data;
>  	int nr_to_scan = sc->nr_to_scan;
> -	int ret, nr_shrunk;
> +	int ret, nr_shrunk, nr_scanned;
>  
>  	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, NULL, &nr_scanned);
> +	sc->nr_scanned = nr_scanned;


I'm a bit concerned that modifying sc->nr_scanned here could actually
end up increasing the number of scan_objects() calls in real-world
usage. If there are other background processes running and continuously
accessing certain files, they may keep producing a small number of new
reclaimable extent caches. That would mean nr_scanned stays at a
relatively small value on each iteration and never drops to 0, so
shrinker->scan_objects() may end up going through more loops because it
can't return SHRINK_STOP. So I'd tend to leave shrinkctl->nr_scanned
alone. What do you think?


>  
>  	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);
> +
> +	/*
> +	 * nr_scanned == 0 does not necessarily mean sbi->s_es_list is empty:
> +	 * es_stats_shk_cnt is a percpu counter and can report a stale/
> +	 * approximate value that is still positive after the list has
> +	 * actually drained, but nr_scanned also stays 0 when every inode
> +	 * __es_shrink() walked this call was momentarily skipped (precached,
> +	 * or lock contended) or had nothing currently shrinkable
> +	 * (es_reclaim_extents() returns without touching nr_to_scan whenever
> +	 * ei->i_es_shk_nr is 0, which can persist for a given inode
> +	 * independent of locking).
> +	 *
> +	 * Treat all of these the same and return SHRINK_STOP rather than
> +	 * only doing so for a genuinely empty list: retrying instead, on
> +	 * the assumption that the other cases are transient, does not
> +	 * decrement do_shrink_slab()'s own scan budget when nr_scanned
> +	 * stays 0, so it never terminates. Without SHRINK_STOP here,
> +	 * do_shrink_slab() also has no way to tell "nothing was there" from
> +	 * "nothing was scanned yet" 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.
> +	 */

This comment seems a bit verbose, could it be simplified a bit?

Thanks,
Yi.

> +	if (nr_scanned == 0)
> +		return SHRINK_STOP;
> +
>  	return nr_shrunk;
>  }
>  
> 
> ---
> base-commit: 502d801f0ab03e4f32f9a33d203154ce84887921
> change-id: 20260928-fix-ext4-es-scan-nr-scanned-5af70744f6cd
> 
> Best regards,

Re: [PATCH v2] ext4: fix shrinker scan budget accounting in ext4_es_scan()

[Jan Kara]

On Wed 30-09-26 09:45:26, Zhang Yi wrote:
> On 9/29/2026 10:20 AM, Qiliang Yuan wrote:
> > @@ -1774,15 +1777,41 @@ static unsigned long ext4_es_scan(struct shrinker *shrink,
> >  {
> >  	struct ext4_sb_info *sbi = shrink->private_data;
> >  	int nr_to_scan = sc->nr_to_scan;
> > -	int ret, nr_shrunk;
> > +	int ret, nr_shrunk, nr_scanned;
> >  
> >  	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, NULL, &nr_scanned);
> > +	sc->nr_scanned = nr_scanned;
> 
> 
> I'm a bit concerned that modifying sc->nr_scanned here could actually
> end up increasing the number of scan_objects() calls in real-world
> usage. If there are other background processes running and continuously
> accessing certain files, they may keep producing a small number of new
> reclaimable extent caches. That would mean nr_scanned stays at a
> relatively small value on each iteration and never drops to 0, so
> shrinker->scan_objects() may end up going through more loops because it
> can't return SHRINK_STOP. So I'd tend to leave shrinkctl->nr_scanned
> alone. What do you think?

Well, I agree but lying to upper layers about the number of objects we have
scanned doesn't look like a proper solution? We could return SHRINK_STOP
when we scanned everything before nr_to_walk dropped to 0. It isn't perfect
but would somewhat address your concern...

								Honza

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

Re: [PATCH v2] ext4: fix shrinker scan budget accounting in ext4_es_scan()

[Zhang Yi]

On 9/30/2026 7:48 PM, Jan Kara wrote:
> On Wed 30-09-26 09:45:26, Zhang Yi wrote:
>> On 9/29/2026 10:20 AM, Qiliang Yuan wrote:
>>> @@ -1774,15 +1777,41 @@ static unsigned long ext4_es_scan(struct shrinker *shrink,
>>>   {
>>>   	struct ext4_sb_info *sbi = shrink->private_data;
>>>   	int nr_to_scan = sc->nr_to_scan;
>>> -	int ret, nr_shrunk;
>>> +	int ret, nr_shrunk, nr_scanned;
>>>   
>>>   	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, NULL, &nr_scanned);
>>> +	sc->nr_scanned = nr_scanned;
>>
>>
>> I'm a bit concerned that modifying sc->nr_scanned here could actually
>> end up increasing the number of scan_objects() calls in real-world
>> usage. If there are other background processes running and continuously
>> accessing certain files, they may keep producing a small number of new
>> reclaimable extent caches. That would mean nr_scanned stays at a
>> relatively small value on each iteration and never drops to 0, so
>> shrinker->scan_objects() may end up going through more loops because it
>> can't return SHRINK_STOP. So I'd tend to leave shrinkctl->nr_scanned
>> alone. What do you think?
> 
> Well, I agree but lying to upper layers about the number of objects we have
> scanned doesn't look like a proper solution? We could return SHRINK_STOP
> when we scanned everything before nr_to_walk dropped to 0. It isn't perfect
> but would somewhat address your concern...
> 
> 								Honza
> 

Yeah, that sounds good to me.

Thanks,
Yi.