On Mon, Jun 06, 2022 at 04:21:36PM +0200, Jan Kara wrote:
> On Mon 06-06-22 12:42:02, Jan Kara wrote:
> > On Thu 02-06-22 10:53:29, Damien Le Moal wrote:
> > > But to avoid the performance regression you observed, we really need to be 100%
> > > sure that all bios have their ->bi_ioprio field correctly initialized. Something
> > > like:
> > >
> > > void bio_set_effective_ioprio(struct *bio)
> > > {
> > > switch (IOPRIO_PRIO_CLASS(bio->bi_ioprio)) {
> > > case IOPRIO_CLASS_RT:
> > > case IOPRIO_CLASS_BE:
> > > case IOPRIO_CLASS_IDLE:
> > > /*
> > > * the bio ioprio was already set from an aio kiocb ioprio
> > > * (aio->aio_reqprio) or from the issuer context ioprio if that
> > > * context used ioprio_set().
> > > */;
> > > return;
> > > case IOPRIO_CLASS_NONE:
> > > default:
> > > /* Use the current task CPU priority */
> > > bio->ioprio =
> > > IOPRIO_PRIO_VALUE(task_nice_ioclass(current),
> > > task_nice_ioprio(current));
> > > return;
> > > }
> > > }
> > >
> > > being called before a bio is inserted in a scheduler or bypass inserted in the
> > > dispatch queues should result in all BIOs having an ioprio that is set to
> > > something other than IOPRIO_CLASS_NONE. And the obvious place may be simply at
> > > the beginning of submit_bio(), before submit_bio_noacct() is called.
> > >
> > > I am tempted to argue that block device drivers should never see any req
> > > with an ioprio set to IOPRIO_CLASS_NONE, which means that no bio should
> > > ever enter the block stack with that ioprio either. With the above
> > > solution, bios from DM targets submitted with submit_bio_noacct() could
> > > still have IOPRIO_CLASS_NONE... So would submit_bio_noacct() be the
> > > better place to call the effective ioprio helper ?
> >
> > Yes, I also think it would be the cleanest if we made sure bio->ioprio is
> > always set to some value other than IOPRIO_CLASS_NONE. I'll see how we can
> > make that happen in the least painful way :). Thanks for your input!
>
> When looking into this I've hit a snag: ioprio rq_qos policy relies on the
> fact that bio->bi_ioprio remains at 0 (unless explicitely set to some other
> value by userspace) until we call rq_qos_track() in blk_mq_submit_bio().
> BTW this happens after we have attempted to merge the bio to existing
> requests so ioprio rq_qos policy is going to show strange behavior wrt
> merging - most of the bios will not be able to merge to existing queued
> requests due to ioprio mismatch.
>
> I'd say .track hook gets called too late to properly set bio->bi_ioprio.
> Shouldn't we set the io priority much earlier - I'd be tempted to use
> bio_associate_blkg_from_css() for this... What do people think?
Hello Jan,
bio_associate_blkg_from_css() is just an empty stub if CONFIG_BLK_CGROUP
is not set.
Having the effective ioprio set should correctly shouldn't depend on if
CONFIG_BLK_CGROUP is set or not, no?
The function name bio_associate_blkg_from_css() (css - cgroup_subsys_state)
also seems to imply that it should only perform cgroup related things, no?
AFAICT, both bfq and mq-deadline can currently prioritize requests without
CONFIG_BLK_CGROUP enabled.
Kind regards,
Niklas
