Re: [PATCH V2 5/7] block: throttle split bio in case of iops limit

[Ming Lei <ming.lei@redhat.com>]

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:
> > 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.
> 
> > 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 <tj@kernel.org>
> 
> for the blk-throtl patches.

Thanks!

-- 
Ming