linux-kernel - Re: [PATCH] blk-cgroup: don't clear stat in blkcg_reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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