On Tue 07-06-22 12:13:48, Niklas Cassel wrote:
> 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.
Correct on all points. However the ioprio rq_qos policy very much depends
on the cgroup support. So at least the update of bio->bi_ioprio based on
that policy would make sense in bio_associate_blkg_from_css(). OTOH
thinking about it now, we would have a problem that if bio blkcg
association changes, we don't have enough information to update the
bi_ioprio accordingly (since we don't know whether the resulting ioprio is
set by userspace or by ioprio rq_qos policy). So probably we need make
ioprio rq_qos policy to set the priority later (likely at bio submission
time when bio blkcg association is stable) but we need to do it earlier
than after we try to merge the bio...
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
