lkml.org 
[lkml]   [2022]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio
From
Date
Hi, Tejun

在 2022/08/17 3:37, Tejun Heo 写道:
> On Tue, Aug 02, 2022 at 10:04:07PM +0800, Yu Kuai wrote:
> ...
>> commit 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
>> support to count splited bios for iops limit, thus it adds flaged bio
> ^
> flagged
>
>> checking in tg_with_in_bps_limit() so that splited bios will only count
> ^
> split
>
>> once for bps limit, however, it introduce a new problem that io throttle
>> won't work if multiple bios are throttled.
>>
>> In order to fix the problem, at first, don't skip flaged bio in
>> tg_with_in_bps_limit(), however, this will break that splited bios should
>> only count once for bps limit. And this patch tries to avoid
>> over-accounting by decrementing it first in __blk_throtl_bio(), and
>> then counting it again while dispatching it.
>>
>> Fixes: 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> Reviewed-by: Ming Lei <ming.lei@redhat.com>
>
> Please cc stable w/ version tag.
>
>> ---
>> block/blk-throttle.c | 26 ++++++++++++++++++++------
>> 1 file changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
>> index 9f5fe62afff9..2957e2c643f4 100644
>> --- a/block/blk-throttle.c
>> +++ b/block/blk-throttle.c
>> @@ -811,7 +811,7 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
>> unsigned int bio_size = throtl_bio_data_size(bio);
>>
>> /* no need to throttle if this bio's bytes have been accounted */
>> - if (bps_limit == U64_MAX || bio_flagged(bio, BIO_THROTTLED)) {
>> + if (bps_limit == U64_MAX) {
>> if (wait)
>> *wait = 0;
>> return true;
>> @@ -921,11 +921,8 @@ static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio)
>> unsigned int bio_size = throtl_bio_data_size(bio);
>>
>> /* Charge the bio to the group */
>> - if (!bio_flagged(bio, BIO_THROTTLED)) {
>> - tg->bytes_disp[rw] += bio_size;
>> - tg->last_bytes_disp[rw] += bio_size;
>> - }
>> -
>> + tg->bytes_disp[rw] += bio_size;
>> + tg->last_bytes_disp[rw] += bio_size;
>> tg->io_disp[rw]++;
>> tg->last_io_disp[rw]++;
>>
>> @@ -2121,6 +2118,23 @@ bool __blk_throtl_bio(struct bio *bio)
>> tg->last_low_overflow_time[rw] = jiffies;
>> throtl_downgrade_check(tg);
>> throtl_upgrade_check(tg);
>> +
>> + /*
>> + * Splited bios can be re-entered because iops limit should be
> ^ ^^^^^^^^^^^^^
> Split re-enter
>
>> + * counted again, however, bps limit should not. Since bps limit
>> + * will be counted again while dispatching it, compensate the
>> + * over-accounting here. Noted that compensation can fail if
>> + * new slice is started.
>
> I can't really follow the comment. Please improve the explanation.
>
>> + */
>> + if (bio_flagged(bio, BIO_THROTTLED)) {
>> + unsigned int bio_size = throtl_bio_data_size(bio);
>> +
>> + if (tg->bytes_disp[rw] >= bio_size)
>> + tg->bytes_disp[rw] -= bio_size;
>> + if (tg->last_bytes_disp[rw] >= bio_size)
>> + tg->last_bytes_disp[rw] -= bio_size;
>> + }
>
> So, as a fix for the immediate problem, I guess this might do but this feels
> really fragile. How can we be certain that re-entering only happens because
> of splitting? What if future core development changes that? It seems to be
> solving the problem in the wrong place. Shouldn't we flag the bio indicating
> that it's split when we're splitting the bio so that we only limit them for
> iops in the first place?
>
Splited bio is tracked in __bio_clone:

if (bio_flagged(bio_src, BIO_THROTTLED))
bio_set_flag(bio, BIO_THROTTLED);

And currenty, the iops limit and bps limit are treated differently,
however there are only one flag 'BIO_THROTTLED' and they can't be
distinguished.

Perhaps I can use two flags, for example BIO_IOPS_THROTTLED and
BIO_BPS_THROTTLED, this way only iops limit can be handled and bps
limit can be skipped for splited bio.

What do you think?

Thanks,
Kuai
> Thanks.
>

\
 
 \ /
  Last update: 2022-08-17 03:14    [W:0.092 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site