[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c3303c93-bb66-c267-4e00-3a19a1ce01a0@huaweicloud.com>
Date: Fri, 28 Jun 2024 11:22:38 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Waiman Long <longman@...hat.com>, Li Lingfeng
<lilingfeng@...weicloud.com>, tj@...nel.org, josef@...icpanda.com,
hch@....de, axboe@...nel.dk
Cc: ming.lei@...hat.com, cgroups@...r.kernel.org,
linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
yangerkun@...wei.com, yukuai1@...weicloud.com, houtao1@...wei.com,
yi.zhang@...wei.com, lilingfeng3@...wei.com, jack@...e.cz,
Paolo VALENTE <paolo.valente@...more.it>, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH] blk-cgroup: don't clear stat in blkcg_reset_stats()
Hi,
+CC Jan and Paolo for bfq part.
在 2024/06/28 5:03, Waiman Long 写道:
>
> On 6/27/24 05:08, Li Lingfeng wrote:
>> From: Li Lingfeng <lilingfeng3@...wei.com>
>>
>> The list corruption described in commit 6da668063279 ("blk-cgroup: fix
>> list corruption from resetting io stat") has no effect. It's unnecessary
>> to fix it.
>>
>> As for cgroup v1, it does not use iostat any more after commit
>> ad7c3b41e86b("blk-throttle: Fix io statistics for cgroup v1"), so using
>> memset to clear iostat has no real effect.
>> As for cgroup v2, it will not call blkcg_reset_stats() to corrupt the
>> list.
>>
>> The list of root cgroup can be used by both cgroup v1 and v2 while
>> non-root cgroup can't since it must be removed before switch between
>> cgroup v1 and v2.
>> So it may has effect if the list of root used by cgroup v2 was corrupted
>> after switching to cgroup v1, and switch back to cgroup v2 to use the
>> corrupted list again.
>> However, the root cgroup will not use the list any more after commit
>> ef45fe470e1e("blk-cgroup: show global disk stats in root cgroup
>> io.stat").
>>
>> Although this has no negative effect, it is not necessary. Remove the
>> related code.
> You may be right that it may not be necessary in the mainline kernel, it
> does fix the issue on distros with older kernels or some stable releases
> where commit ad7c3b41e86b("blk-throttle: Fix io statistics for cgroup
> v1") may not be present.
>
>>
>> Fixes: 6da668063279 ("blk-cgroup: fix list corruption from resetting
>> io stat")
> I don't think there should be a fixes tag or it will be backported to
> stable releases.
Yes, and probably it's better to add:
Depends-on: ad7c3b41e86b ("blk-throttle: Fix io statistics for cgroup v1")
Depends-on: 0416f3be58c6 ("blk-cgroup: don't update io stat for root
cgroup")
>> Signed-off-by: Li Lingfeng <lilingfeng3@...wei.com>
>> ---
>> block/blk-cgroup.c | 24 ------------------------
>> 1 file changed, 24 deletions(-)
>>
>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index 37e6cc91d576..1113c398a742 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -629,29 +629,6 @@ static void blkg_iostat_set(struct blkg_iostat
>> *dst, struct blkg_iostat *src)
>> }
>> }
>> -static void __blkg_clear_stat(struct blkg_iostat_set *bis)
>> -{
>> - struct blkg_iostat cur = {0};
>> - unsigned long flags;
>> -
>> - flags = u64_stats_update_begin_irqsave(&bis->sync);
>> - blkg_iostat_set(&bis->cur, &cur);
>> - blkg_iostat_set(&bis->last, &cur);
>> - u64_stats_update_end_irqrestore(&bis->sync, flags);
>> -}
>> -
>> -static void blkg_clear_stat(struct blkcg_gq *blkg)
>> -{
>> - int cpu;
>> -
>> - for_each_possible_cpu(cpu) {
>> - struct blkg_iostat_set *s = per_cpu_ptr(blkg->iostat_cpu, cpu);
>> -
>> - __blkg_clear_stat(s);
>> - }
>> - __blkg_clear_stat(&blkg->iostat);
>> -}
>> -
>> static int blkcg_reset_stats(struct cgroup_subsys_state *css,
>> struct cftype *cftype, u64 val)
>> {
>> @@ -668,7 +645,6 @@ static int blkcg_reset_stats(struct
>> cgroup_subsys_state *css,
>> * anyway. If you get hit by a race, retry.
>> */
>> hlist_for_each_entry(blkg, &blkcg->blkg_list, blkcg_node) {
>> - blkg_clear_stat(blkg);
>> for (i = 0; i < BLKCG_MAX_POLS; i++) {
>> struct blkcg_policy *pol = blkcg_policy[i];
>
> If you are saying that iostat is no longer used in cgroup v1, why not
> remove the blkcg_reset_stats() and its supporting functions and
> deprecate the v1 reset_stats control file. The file should still be
> there to avoid userspace regression, but it will be a nop.
No, this is not correct, blkg->iostat is not used in cgroup v1, however,
bfq(and probably blk-throtl) has it's only stats, and they rely on
pd_reset_stats_fn() from blkcg_reset_stats() to clear the stats. I don't
think remove it is a good idea for now, bfq maintainer must agree with
this.
Thanks,
Kuai
>
> Cheers,
> Longman
>
>
> .
>
Powered by blists - more mailing lists