On Mon, Feb 14, 2022 at 10:22:49AM -1000, Tejun Heo wrote:

Hello,

On Wed, Feb 09, 2022 at 05:14:27PM +0800, Ming Lei wrote:

quoted

Chunguang Xu has already observed this issue and fixed it in commit
4f1e9630afe6 ("blk-throtl: optimize IOPS throttle for large IO scenarios").
However, that patch only covers bio splitting in __blk_queue_split(), and
we have other kind of bio splitting, such as bio_split() &
submit_bio_noacct() and other ways.

I see. So, we can go for adding split handling to all other places or try to
find a common spot.

quoted

This patch tries to fix the issue in one generic way by always charging
the bio for iops limit in blk_throtl_bio(). This way is reasonable:
re-submission & fast-cloned bio is charged if it is submitted to same
disk/queue, and BIO_THROTTLED will be cleared if bio->bi_bdev is changed.

This new approach can get much more smooth/stable iops limit compared with
commit 4f1e9630afe6 ("blk-throtl: optimize IOPS throttle for large IO
scenarios") since that commit can't throttle current split bios actually.

Also this way won't cause new double bio iops charge in
blk_throtl_dispatch_work_fn() in which blk_throtl_bio() won't be called
any more.

But yeah, this should work too. This is simpler but more fragile given the
twisted history around BIO_THROTTLED. I think the root cause of the
convolution is that it's hooked at the wrong spot - it's sitting on top of
the queue trying to guess what actually happens to the bios it sent down.
Ideally, we probably wanna move this to rq-qos hooks which sit on the actual
issue-to-the-device path.

The big problem is that rq-qos is only hooked for request based queue,
and we need to support cgroup/throttle for bio base queue.

For now, I don't have a strong preference. This looks fine to me too. Please
feel free to add

 Acked-by: Tejun Heo [off-list ref]

for the blk-throtl patches.

Thanks!

-- 
Ming