[PATCH] block: elevator.c: Check elevator kernel argument again

[PATCH] block: elevator.c: Check elevator kernel argument again

[Marcos Paulo de Souza]

Since the inclusion of blk-mq, elevator= kernel argument was not being
considered anymore, making it impossible to specify a specific elevator
at boot time as it was used before.

This is done by checking chosen_elevator global variable, which is
populated once elevator= kernel argument is passed. Without this patch,
mq-deadline is the only elevator that is can be used at boot time.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---

 I found this issue while inspecting why noop scheduler was gone, and so I found
 that was now impossible to use a scheduler different from mq-deadeline.

 Am I missing something? Is this a desirable behavior?

 One more question: currently we can't specify a "none" scheduler, like it used
 to be "noop". This is also on purpose? If it's not, I can provide a patch for
 it.

 block/elevator.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index 2f17d66d0e61..41ce7ba099ba 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -601,7 +601,7 @@ int elevator_switch_mq(struct request_queue *q,
  */
 int elevator_init_mq(struct request_queue *q)
 {
-	struct elevator_type *e;
+	struct elevator_type *e = NULL;
 	int err = 0;
 
 	if (q->nr_hw_queues != 1)
@@ -615,9 +615,18 @@ int elevator_init_mq(struct request_queue *q)
 	if (unlikely(q->elevator))
 		goto out_unlock;
 
-	e = elevator_get(q, "mq-deadline", false);
-	if (!e)
-		goto out_unlock;
+	/* if elevator was used as kernel argument, try to load it */
+	if (*chosen_elevator) {
+		e = elevator_get(q, chosen_elevator, false);
+		if (!e)
+			pr_err("io scheduler %s not found", chosen_elevator);
+	}
+
+	if (!e) {
+		e = elevator_get(q, "mq-deadline", false);
+		if (!e)
+			goto out_unlock;
+	}
 
 	err = blk_mq_init_sched(q, e);
 	if (err)
-- 
2.22.0

Re: [PATCH] block: elevator.c: Check elevator kernel argument again

[Jens Axboe]

On 7/12/19 9:52 PM, Marcos Paulo de Souza wrote:
> Since the inclusion of blk-mq, elevator= kernel argument was not being
> considered anymore, making it impossible to specify a specific elevator
> at boot time as it was used before.
> 
> This is done by checking chosen_elevator global variable, which is
> populated once elevator= kernel argument is passed. Without this patch,
> mq-deadline is the only elevator that is can be used at boot time.
> 
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
> ---
> 
>   I found this issue while inspecting why noop scheduler was gone, and
>   so I found that was now impossible to use a scheduler different from
>   mq-deadeline.
> 
>   Am I missing something? Is this a desirable behavior?

Just google, I'm sure 2-3 discussions on this topic will come up.

tldr is that the original parameter was a mistake and doesn't work at
all for multiple devices. Today it's even worse, as we have device
types that won't even work properly with any scheduler, liked the
zoned devices. The parameter was never enabled for blk-mq because of
that, and hence died when the legacy IO path was scrapped. It's not
coming back.

-- 
Jens Axboe

Re: [PATCH] block: elevator.c: Check elevator kernel argument again

[Marcos Paulo de Souza]

On Sat, Jul 13, 2019 at 03:53:28PM -0600, Jens Axboe wrote:
> On 7/12/19 9:52 PM, Marcos Paulo de Souza wrote:
> > Since the inclusion of blk-mq, elevator= kernel argument was not being
> > considered anymore, making it impossible to specify a specific elevator
> > at boot time as it was used before.
> > 
> > This is done by checking chosen_elevator global variable, which is
> > populated once elevator= kernel argument is passed. Without this patch,
> > mq-deadline is the only elevator that is can be used at boot time.
> > 
> > Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
> > ---
> > 
> >   I found this issue while inspecting why noop scheduler was gone, and
> >   so I found that was now impossible to use a scheduler different from
> >   mq-deadeline.
> > 
> >   Am I missing something? Is this a desirable behavior?
> 
> Just google, I'm sure 2-3 discussions on this topic will come up.
> 
> tldr is that the original parameter was a mistake and doesn't work at
> all for multiple devices. Today it's even worse, as we have device
> types that won't even work properly with any scheduler, liked the
> zoned devices. The parameter was never enabled for blk-mq because of
> that, and hence died when the legacy IO path was scrapped. It's not
> coming back.

Thanks Jens. So it makes sense to remove all leftover code of elevator argument with
some dead documentation about it, avoiding confusion about it in the future
again. I will send some patches tomorrow.

> 
> -- 
> Jens Axboe
>