[PATCH] block/blk-stat: fix mean loss when re-summing aggregated stats

[PATCH] block/blk-stat: fix mean loss when re-summing aggregated stats

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

blk_rq_stat_sum() folds src into dst but only advances dst->mean and
dst->nr_samples, leaving dst->batch untouched.  The mean is computed
from src->batch (the raw per-cpu sum), so a stat that has already been
through one sum carries batch=0; if that aggregated stat is then used
as the src of another sum, its samples add nothing to the new mean.

iolatency hits exactly that: iolatency_check_latencies() first sums the
per-cpu stats into a local stat, then sums that local stat into
iolat->cur_stat.  After the first sum the local stat has batch=0, so
every later window drives cur_stat->mean toward zero.  On non-SSD
devices it stays at 0, making the latency_sum_ok(&cur_stat) check that
gates scaling up always true -- the scale-up hysteresis is effectively
defeated.  SSD devices use the percentile path and are unaffected.

Keep dst->batch in sync across sums so an aggregated stat can be reused
as a src.  blk_rq_stat.batch is internal to blk_rq_stat_init/_add/_sum
(no other reader in the tree), so the wbt and blk-mq consumers, which
only read ->mean/->min/->nr_samples, behave as before.

Fixes: 34dbad5d26e2 ("blk-stat: convert to callback-based statistics reporting")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>

---
 block/blk-stat.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/blk-stat.c b/block/blk-stat.c
index de126e1ea5ac..4d4781350083 100644
--- a/block/blk-stat.c
+++ b/block/blk-stat.c
@@ -36,6 +36,7 @@ void blk_rq_stat_sum(struct blk_rq_stat *dst, struct blk_rq_stat *src)
 	dst->mean = div_u64(src->batch + dst->mean * dst->nr_samples,
 				dst->nr_samples + src->nr_samples);
 
+	dst->batch += src->batch;
 	dst->nr_samples += src->nr_samples;
 }
 
--
2.43.0

Re: [PATCH] block/blk-stat: fix mean loss when re-summing aggregated stats

[Tang Yizhou]

On 20/7/26 9:38 pm, Tao Cui wrote:
> From: Tao Cui <cuitao@kylinos.cn>
> 
> blk_rq_stat_sum() folds src into dst but only advances dst->mean and
> dst->nr_samples, leaving dst->batch untouched.  The mean is computed
> from src->batch (the raw per-cpu sum), so a stat that has already been
> through one sum carries batch=0; if that aggregated stat is then used
> as the src of another sum, its samples add nothing to the new mean.
> 
> iolatency hits exactly that: iolatency_check_latencies() first sums the
> per-cpu stats into a local stat, then sums that local stat into
> iolat->cur_stat.  After the first sum the local stat has batch=0, so
> every later window drives cur_stat->mean toward zero.  On non-SSD

Good catch, this is indeed a real issue.

> devices it stays at 0, making the latency_sum_ok(&cur_stat) check that
> gates scaling up always true -- the scale-up hysteresis is effectively
> defeated.  SSD devices use the percentile path and are unaffected.
> 
> Keep dst->batch in sync across sums so an aggregated stat can be reused
> as a src.  blk_rq_stat.batch is internal to blk_rq_stat_init/_add/_sum
> (no other reader in the tree), so the wbt and blk-mq consumers, which
> only read ->mean/->min/->nr_samples, behave as before.
> 
> Fixes: 34dbad5d26e2 ("blk-stat: convert to callback-based statistics reporting")
> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> 
> ---
>  block/blk-stat.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/blk-stat.c b/block/blk-stat.c
> index de126e1ea5ac..4d4781350083 100644
> --- a/block/blk-stat.c
> +++ b/block/blk-stat.c
> @@ -36,6 +36,7 @@ void blk_rq_stat_sum(struct blk_rq_stat *dst, struct blk_rq_stat *src)
>  	dst->mean = div_u64(src->batch + dst->mean * dst->nr_samples,
>  				dst->nr_samples + src->nr_samples);
>  
> +	dst->batch += src->batch;

This change doesn't actually fix the issue. As you already found, the local
stat's batch is 0. Also, according to the comment of blk_rq_stat_sum(), @src is
a per-CPU stat, so there is no issue in blk_stat_timer_fn(). Your change would
actually make this case look strange.

-- 
Best Regards,
Yi

>  	dst->nr_samples += src->nr_samples;
>  }
>  
> --
> 2.43.0
> 

Re: [PATCH] block/blk-stat: fix mean loss when re-summing aggregated stats

[Tao Cui]

在 2026/7/21 01:05, Tang Yizhou 写道:
> On 20/7/26 9:38 pm, Tao Cui wrote:
>> From: Tao Cui <cuitao@kylinos.cn>
>>
>> blk_rq_stat_sum() folds src into dst but only advances dst->mean and
>> dst->nr_samples, leaving dst->batch untouched.  The mean is computed
>> from src->batch (the raw per-cpu sum), so a stat that has already been
>> through one sum carries batch=0; if that aggregated stat is then used
>> as the src of another sum, its samples add nothing to the new mean.
>>
>> iolatency hits exactly that: iolatency_check_latencies() first sums the
>> per-cpu stats into a local stat, then sums that local stat into
>> iolat->cur_stat.  After the first sum the local stat has batch=0, so
>> every later window drives cur_stat->mean toward zero.  On non-SSD
> 
> Good catch, this is indeed a real issue.
> 
>> devices it stays at 0, making the latency_sum_ok(&cur_stat) check that
>> gates scaling up always true -- the scale-up hysteresis is effectively
>> defeated.  SSD devices use the percentile path and are unaffected.
>>
>> Keep dst->batch in sync across sums so an aggregated stat can be reused
>> as a src.  blk_rq_stat.batch is internal to blk_rq_stat_init/_add/_sum
>> (no other reader in the tree), so the wbt and blk-mq consumers, which
>> only read ->mean/->min/->nr_samples, behave as before.
>>
>> Fixes: 34dbad5d26e2 ("blk-stat: convert to callback-based statistics reporting")
>> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
>>
>> ---
>>  block/blk-stat.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/block/blk-stat.c b/block/blk-stat.c
>> index de126e1ea5ac..4d4781350083 100644
>> --- a/block/blk-stat.c
>> +++ b/block/blk-stat.c
>> @@ -36,6 +36,7 @@ void blk_rq_stat_sum(struct blk_rq_stat *dst, struct blk_rq_stat *src)
>>  	dst->mean = div_u64(src->batch + dst->mean * dst->nr_samples,
>>  				dst->nr_samples + src->nr_samples);
>>  
>> +	dst->batch += src->batch;
> 
> This change doesn't actually fix the issue. As you already found, the local
> stat's batch is 0. Also, according to the comment of blk_rq_stat_sum(), @src is
> a per-CPU stat, so there is no issue in blk_stat_timer_fn(). Your change would
> actually make this case look strange.
> 
You're right. blk_rq_stat_sum() is contracted on a raw per-cpu src,
which blk_stat_timer_fn() honors; the actual misuse is iolatency
feeding an already-aggregated stat at blk-iolatency.c. I'll drop
the blk-stat.c change and fix it on the iolatency side — merging
cur_stat with the per-window stat by their reconstructed totals, since
both already carry a valid mean. Will send a v2.

Thanks,
Tao