Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6128380c-b148-cb7e-44d5-0bd7d05a2942@bytedance.com>
Date:   Fri, 31 Mar 2023 11:00:33 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.0
Subject: Re: [External] Re: [PATCH] blk-throttle: Fix io statistics for cgroup
 v1
To:     Tejun Heo <tj@kernel.org>
Cc:     josef@toxicpanda.com, axboe@kernel.dk, cgroups@vger.kernel.org,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230328142309.73413-1-hanjinke.666@bytedance.com>
 <ZCSJaBO8i5jQFC10@slm.duckdns.org>
 <1a858cce-4d87-5e0a-9274-52cffde7dea6@bytedance.com>
 <ZCY7EoAUqfB0ac8S@slm.duckdns.org>
From:   hanjinke <hanjinke.666@bytedance.com>
In-Reply-To: <ZCY7EoAUqfB0ac8S@slm.duckdns.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 2023/3/31 上午9:44, Tejun Heo 写道:
> Hello,
> 
> On Thu, Mar 30, 2023 at 11:44:04AM +0800, hanjinke wrote:
>> 在 2023/3/30 上午2:54, Tejun Heo 写道:
>>> On Tue, Mar 28, 2023 at 10:23:09PM +0800, Jinke Han wrote:
>>>> From: Jinke Han <hanjinke.666@bytedance.com>
>>>>
>>>> Now the io statistics of cgroup v1 are no longer accurate. Although
>>>> in the long run it's best that rstat is a good implementation of
>>>> cgroup v1 io statistics. But before that, we'd better fix this issue.
>>>
>>> Can you please expand on how the stats are wrong on v1 and how the patch
>>> fixes it?
>>>
>>> Thanks.
>>>
>> Now blkio.throttle.io_serviced and blkio.throttle.io_serviced become the
> 
> "now" might be a bit too vague. Can you point to the commit which made the
> change?
> 
>> only stable io stats interface of cgroup v1, and these statistics are done
>> in the blk-throttle code. But the current code only counts the bios that are
> 
> Ah, okay, so the stats are now updated by blk-throtl itself but
> 
>> actually throttled. When the user does not add the throttle limit, the io
>> stats for cgroup v1 has nothing. I fix it according to the statistical
>> method of v2, and made it count all ios accurately.
> 
> updated only when limits are configured which can be confusing. Makes sense
> to me. Can you please update the patch description accordingly?
> 
> Also, the following change:
> 
> @@ -2033,6 +2033,9 @@ void blk_cgroup_bio_start(struct bio *bio)
>          struct blkg_iostat_set *bis;
>          unsigned long flags;
> 
> +       if (!cgroup_subsys_on_dfl(io_cgrp_subsys))
> +               return;
> +
>          /* Root-level stats are sourced from system-wide IO stats */
>          if (!cgroup_parent(blkcg->css.cgroup))
>                  return;
> 
> seems incomplete as there's an additional
> cgroup_subsys_on_dfl(io_cgrp_subsys) test in the function. We probably wanna
> remove that?
> 
> Thanks.
> 

okay, according to your suggestion, I will send a v2.

Thanks