Re: [PATCH RFC v2 6/6] blk-throtl: switch to use rq_qos

[Yu Kuai]

Hi,

在 2024/04/13 10:17, Yu Kuai 写道:
> Hi,
>
> 在 2024/04/13 2:11, Tejun Heo 写道:
> > Hello,
> >
> > On Sat, Apr 06, 2024 at 04:00:59PM +0800, Yu Kuai wrote:
> > > From: Yu Kuai <yukuai3@xxxxxxxxxx>
> > >
> > > To avoid exposing blk-throttle internal implementation to general block
> > > layer.
> > ...
> > > @@ -832,7 +832,7 @@ void submit_bio_noacct(struct bio *bio)
> > >           goto not_supported;
> > >       }
> > > -    if (blk_throtl_bio(bio))
> > > +    if (rq_qos_throttle_bio(q, bio))
> > >           return;
> > >       submit_bio_noacct_nocheck(bio);
> > >       return;
> >
> > This is a half-way conversion, right? You're adding a dedicated hook to
> > rq_qos and none of the other hooks can be used by blk-throtl. Even the 
> > name,

Actually, rq_qos_exit() is used as well for destroy blk-throtl.

> > rq_qos_throttle_bio(), becomes a misnomer. I'm not really sure this makes
> > things better or worse. It makes certain things a bit cleaner but other
> > things nastier. I don't know.
>
> Yes, the final goal is making all blk-cgroup policies modular, and this
> patch use rq-qos to prevent exposing blk-throtle to block layer, like
> other policies.

After thinking this a bit more, I still think probably rq_qos is a
better choice, and there is something that I want to discuss.

There are two different direction, first is swith blk-throttle to
rq_qos_throttle() as well, which is called for each rq:

1) For, rq-based device, why blk-throtl must throttle before
rq_qos_throttle()? And blk-throtl have to handle the bio split case
seperately. And it looks like blk-throttle can switch to use
rq_qos_throttle() with the benefit that io split does't need
special handling.

2) blk-throtl treats split IO as additional iops, while it ignores
merge IO, this looks wrong to me. If multiple bio merged into one
request, iostat will see just one IO. And after switching to rq_qos,
bio merge case can be handled easily as well.

Another is still add a rq_qos_throttle_bio(perhaps another name?), and
meanwhile iocost can benefit from this new helper as well. Because
iocost really is based on bio, currently it must handle the io merge
case by debt.

Thanks,
Kuai

> There is another choice that I think is feasible:
>
> Let blk-throttle ping a policy id, and use the id to call throttle
> function directly, this will require initializing the 'plid' from
> blkcg_policy() during definition instead of blkcg_policy_register().
>
> Thanks,
> Kuai
>
> > Thanks.
>
> .