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?
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
