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

...

> 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@xxxxxxxx>
SUSE Labs, CR