Re: [PATCH] quota: fix shrinker scan budget accounting in dqcache_shrink_scan()

[Jan Kara]

On Mon 28-09-26 20:54:40, Qiliang Yuan wrote:
> The dquot cache shrinker's count_objects() callback,
> dqcache_shrink_count(), reports the number of unused dquot objects
> via the DQST_FREE_DQUOTS percpu counter (through vfs_pressure_ratio()).
> 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.
> 
> dqcache_shrink_scan() never updates sc->nr_scanned, even though
> include/linux/shrinker.h documents that "the callee should track its
> actual progress" in that field. The function already decrements
> sc->nr_to_scan in place as it walks free_dquots, one per dquot
> destroyed, and its while loop exits immediately without touching
> sc->nr_to_scan at all when free_dquots is already empty. 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 actually happened, so it has no way to tell "there was
> nothing to do this pass" from "a full batch was scanned and none of
> it was freeable", and keeps calling scan_objects() until the
> original, one-shot total_scan budget computed from the percpu count
> is drained, even though free_dquots emptied out long before that
> budget was used up.
> 
> Compute sc->nr_scanned from the difference between the nr_to_scan
> value on entry and the value left behind by the reclaim loop, and
> return SHRINK_STOP once that difference comes back as zero, since
> that only happens when free_dquots was already empty and no further
> calls in this reclaim pass can make progress.
> 
> Tested on a loop-mounted ext4 image with the quota feature enabled:
> chown()ing several hundred files to distinct uid:gid pairs to
> populate allocated dquots, then triggering
> "echo 2 > /proc/sys/vm/drop_caches" to let them drop to zero
> references and become reclaimable, while using an ad hoc kprobe pair
> on dqcache_shrink_scan() (this shrinker has no dedicated tracepoints)
> to record sc->nr_to_scan on entry and the return value on exit:
> 
>                       total scan_objects()   calls with
>                       calls                  freed == 0
>   before this patch     12                    5 (42%)
>   after this patch        5                    0
> 
> The last call in the "after" run returns SHRINK_STOP
> (0xffffffffffffffff) instead of a fifth zero-progress attempt.
> 
> This is the same missing sc->nr_scanned accounting already fixed in
> the analogous ext4 extents_status shrinker and the jbd2 checkpoint
> shrinker; dqcache_shrink_scan() was found by auditing every shrinker
> in the tree whose count_objects() reads a percpu_counter (the
> precondition for count_objects() to report a stale, still-positive
> value after the reclaimable list has actually drained), while ruling
> out the much larger set of shrinkers built on the exact list_lru
> atomic counters, which cannot observe this kind of staleness.
> 
> Signed-off-by: Qiliang Yuan <odys.yuan@xxxxxxxxx>

Thanks. I've added the patch to my tree.

								Honza

> ---
>  fs/quota/dquot.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 9850de3955d31..b55fa7d63f56d 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -809,6 +809,7 @@ static unsigned long
>  dqcache_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
>  {
>  	struct dquot *dquot;
> +	unsigned long orig_nr_to_scan = sc->nr_to_scan;
>  	unsigned long freed = 0;
>  
>  	spin_lock(&dq_list_lock);
> @@ -822,6 +823,21 @@ dqcache_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
>  		freed++;
>  	}
>  	spin_unlock(&dq_list_lock);
> +
> +	sc->nr_scanned = orig_nr_to_scan - sc->nr_to_scan;
> +
> +	/*
> +	 * DQST_FREE_DQUOTS is a percpu counter, so count_objects() can
> +	 * report a stale/approximate value that is still positive even
> +	 * though free_dquots is actually empty by the time we get here.
> +	 * When that happens sc->nr_scanned comes back 0 and we have
> +	 * nothing further to offer this reclaim pass, so tell
> +	 * do_shrink_slab() to stop calling us instead of letting it burn
> +	 * through its one-shot scan budget against an empty list.
> +	 */
> +	if (sc->nr_scanned == 0)
> +		return SHRINK_STOP;
> +
>  	return freed;
>  }
>  
> 
> ---
> base-commit: 502d801f0ab03e4f32f9a33d203154ce84887921
> change-id: 20260928-fix-dquot-shrink-scan-nr-scanned-b11e8550287d
> 
> Best regards,
> -- 
> Qiliang Yuan <odys.yuan@xxxxxxxxx>
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR