Message-ID: <9852111b-cbb2-bb39-652c-5e56f28bd991@huaweicloud.com>
Date: Tue, 20 Aug 2024 15:15:10 +0800
From: Li Lingfeng <lilingfeng@...weicloud.com>
To: Yu Kuai <yukuai1@...weicloud.com>, Tejun Heo <tj@...nel.org>
Cc: josef@...icpanda.com, hch@....de, axboe@...nel.dk, longman@...hat.com,
 ming.lei@...hat.com, cgroups@...r.kernel.org, linux-block@...r.kernel.org,
 linux-kernel@...r.kernel.org, yangerkun@...wei.com, houtao1@...wei.com,
 yi.zhang@...wei.com, lilingfeng3@...wei.com, "yukuai (C)"
 <yukuai3@...wei.com>
Subject: Re: [PATCH] blk-cgroup: don't clear stat in blkcg_reset_stats()

Friendly ping ...

Thank.

在 2024/6/28 11:14, Yu Kuai 写道:
> Hi,
>
> 在 2024/06/28 4:10, Tejun Heo 写道:
>> Hello,
>>
>> On Thu, Jun 27, 2024 at 05:08:56PM +0800, Li Lingfeng wrote:
>>> 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.
>>
>> I find this paragraph a bit confusing. At the time, it was broken, 
>> right?
>> And if we were to memset() now, it'd break again.
>>
>>> 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.
>>
>> Ah, okay, this is because we made the stats blk-throtl specific but 
>> didn't
>> implement ->pd_reset_stat_fn(), right?
>
> I'm afraid not... Implement pd_reset_stat_fn() or not is another
> problem, this patch should be just code cleanup, not fixing any real
> problems.
>
>>
>>> 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").
>>
>> Hmm... I'm still having a bit of trouble following this line of argument
>> given that all the patch does is dropping stat clearing.
>>
>>> @@ -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];
>>
>> The patch looks fine to me although it'd be nice to follow up with a 
>> patch
>> to implement ->pd_reset_stat_fn() for blk-throtl. I'm not quite 
>> following
>> the list corruption part of argument.
>
> The code deleted by this patch was claimed to fix a lsit corruption,
> however, the list corruption does not exist now hence related code can
> be removed:
>
> 1) Take a look at blk_cgroup_bio_start, now there are two conditions
> before this blkg can be added to the per_cpu list:
>
> blk_cgroup_bio_start
>  if (!cgroup_subsys_on_dfl(io_cgrp_subsys))
>  -> the list will always be empty for cgroup v1
>   return;
>  if (!cgroup_parent(blkcg->css.cgroup))
>  -> the list will always be empty for root blkg
>   return;
>  ...
>  llist_add()
>
> 2) blkcg_reset_stats can only be called from cgroup v1 api, hence
> there is nothing to be cleared for blkg_clear_stat();
>
> 3) Noted that user can switch from cgroup v2 to v1, however, we found
> that user must delete all the child cg to do that, hence only root blkg
> can be kept after switching to v1. And root blkg is bypassed from
> blk_cgroup_bio_start(), hence no problem.
>
> Thanks,
> Kuai
>>
>> Thanks.
>>
>

Powered by blists - more mailing lists