On Fri, Apr 11, 2025 at 12:07:34AM +0530, Nilay Shroff wrote:
>
>
> On 4/10/25 7:00 PM, Ming Lei wrote:
> > /*
> > * Use the default elevator settings. If the chosen elevator initialization
> > * fails, fall back to the "none" elevator (no elevator).
> > */
> > -void elevator_init_mq(struct request_queue *q)
> > +void elevator_set_default(struct request_queue *q)
> > {
> > - struct elevator_type *e;
> > - unsigned int memflags;
> > + struct elev_change_ctx ctx = { };
> > int err;
> >
> > - WARN_ON_ONCE(blk_queue_registered(q));
> > -
> > - if (unlikely(q->elevator))
> > + if (!queue_is_mq(q))
> > return;
> >
> > - e = elevator_get_default(q);
> > - if (!e)
> > + ctx.name = use_default_elevator(q) ? "mq-deadline" : "none";
> > + if (!q->elevator && !strcmp(ctx.name, "none"))
> > return;
> > + err = elevator_change(q, &ctx);
> > + if (err < 0)
> > + pr_warn("\"%s\" set elevator failed %d, "
> > + "falling back to \"none\"\n", ctx.name, err);
> > +}
> >
> If we fail to set the evator to default (mq-deadline) while registering queue,
> because nr_hw_queue update is simultaneously running then we may end up setting
> the queue elevator to none and that's not correct. Isn't it?
It still works with none.
I think it isn't one big deal. And if it is really one issue in future, we can
set one flag in elevator_set_default(), and let blk_mq_update_nr_hw_queues set
default sched for us.
>
> > +void elevator_set_none(struct request_queue *q)
> > +{
> > + struct elev_change_ctx ctx = {
> > + .name = "none",
> > + .uevent = 1,
> > + };
> > + int err;
> >
> > - blk_mq_unfreeze_queue(q, memflags);
> > + if (!queue_is_mq(q))
> > + return;
> >
> > - if (err) {
> > - pr_warn("\"%s\" elevator initialization failed, "
> > - "falling back to \"none\"\n", e->elevator_name);
> > - }
> > + if (!q->elevator)
> > + return;
> >
> > - elevator_put(e);
> > + err = elevator_change(q, &ctx);
> > + if (err < 0)
> > + pr_warn("%s: set none elevator failed %d\n", __func__, err);
> > }
> >
> Here as well if we fail to disable/exit elevator while deleting disk
> because nr_hw_queue update is simultaneously running then we may
> leak elevator resource?
When blk_mq_update_nr_hw_queues() observes that queue is dying, it
forces to change elevator to none, so there isn't elevator leak issue.
>
> > @@ -565,11 +559,7 @@ int __must_check add_disk_fwnode(struct device *parent, struct gendisk *disk,
> > if (disk->major == BLOCK_EXT_MAJOR)
> > blk_free_ext_minor(disk->first_minor);
> > out_exit_elevator:
> > - if (disk->queue->elevator) {
> > - mutex_lock(&disk->queue->elevator_lock);
> > - elevator_exit(disk->queue);
> > - mutex_unlock(&disk->queue->elevator_lock);
> > - }
> > + elevator_set_none(disk->queue);
> Same comment as above here as well but this is in add_disk code path.
We can avoid it by forcing to change to none in blk_mq_update_nr_hw_queues() for
!blk_queue_registered()
Thanks,
Ming
