[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6b4baf79-365f-946f-3d71-e78fafbd0988@huaweicloud.com>
Date: Fri, 28 Jun 2024 11:17:49 +0800
From: Li Lingfeng <lilingfeng@...weicloud.com>
To: Waiman Long <longman@...hat.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, paolo.valente@...more.it
Subject: Re: [PATCH] blk-cgroup: don't clear stat in blkcg_reset_stats()
在 2024/6/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.
OK, I will remove it.
>> 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.
I'm not sure if we can just remove blkcg_reset_stats() since
bfq_pd_reset_stats() may be called in it.
Thanks.
>
> Cheers,
> Longman
Powered by blists - more mailing lists