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@xxxxxxxxxx>
>
> for the blk-throtl patches.
Thanks!
--
Ming
