linux-kernel - Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

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 for Android: free password hash cracker in your pocket

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

Message-ID: <b1791e41-4945-a0d8-8067-3841ce10fddd@huaweicloud.com>
Date:   Mon, 22 Aug 2022 11:06:44 +0800
From:   Yu Kuai <yukuai1@...weicloud.com>
To:     Yu Kuai <yukuai1@...weicloud.com>, Tejun Heo <tj@...nel.org>
Cc:     mkoutny@...e.com, axboe@...nel.dk, ming.lei@...hat.com,
        cgroups@...r.kernel.org, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
        "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work
 for single bio

Hi,

在 2022/08/18 9:23, Yu Kuai 写道:
> Hi, Tejun!
> 
> 在 2022/08/18 1:50, Tejun Heo 写道:
>> Hello,
>>
>> On Wed, Aug 17, 2022 at 09:13:38AM +0800, Yu Kuai wrote:
>>>> 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:
>>
>> As the word is used in commit messages and comments, the past perfect 
>> form
>> of the verb "split" is "split". It looks like "splitted" is used in rare
>> cases but dictionary says it's an archaic form.
> 
> Ok, thanks for pointing it out, I'll change that in next iteration.
>>
>>> if (bio_flagged(bio_src, BIO_THROTTLED))
>>>     bio_set_flag(bio, BIO_THROTTLED);

While implementing the new method, I found that there seems to be a
misunderstanding here, the code seems to try to add flag to split bio
so that it won't be throttled again for bps limit, however:

1) for blk throttle, split bio is issued directly and will never be
throttled again, while orignal bio will go through throttle path again.
2) if cloned bio is directed to a new disk, the flag is cleared anyway.
>>>
>>> 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?
>>
>> I think the code would be a lot more intuitive and less fragile if we 
>> used
>> two flags but the bits in the bi_flags field are a scarce resource
>> unfortunately. Even then, I think the right thing to do here is using two
>> flags.
> 
> Yes, the field 'bio->bi_flags' is unsigned short, and there are only two
> bits left. I'll use the new sulution which will acquire a new bit.
> 
> Thanks,
> Kuai
>>
>> Thanks.
>>
> 
> .
>