block: elevator.c: Check elevator kernel argument again
diff mbox series

Message ID 20190713035221.31508-1-marcos.souza.org@gmail.com
State New
Headers show
Series
  • block: elevator.c: Check elevator kernel argument again
Related show

Commit Message

Marcos Paulo de Souza July 13, 2019, 3:52 a.m. UTC
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(-)

Comments

Jens Axboe July 13, 2019, 9:53 p.m. UTC | #1
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.
Marcos Paulo de Souza July 14, 2019, 4:01 a.m. UTC | #2
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
>

Patch
diff mbox series

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)