[PATCH BUGFIX] attempt to fix wrong scheduler selection

[PATCH BUGFIX] attempt to fix wrong scheduler selection

[Paolo Valente]

Hi,
if, at boot, a legacy I/O scheduler is chosen for a device using
blk-mq, or, viceversa, a blk-mq scheduler is chosen for a device using
blk, then that scheduler is set and initialized without any check,
driving the system into an inconsistent state.

The purpose of this message is, first, to report this issue, and,
second, to propose a possible fix in case you do consider this as a
bug.

Thanks,
Paolo

Paolo Valente (1):
  block: make elevator_get robust against cross blk/blk-mq choice

 block/elevator.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

--
2.10.0

[PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice

[Paolo Valente]

If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq,
or, viceversa, a blk-mq scheduler is chosen for a device using blk, then
that scheduler is set and initialized without any check, driving the
system into an inconsistent state. This commit addresses this issue by
letting elevator_get fail for these wrong cross choices.

Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/elevator.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index 27ff1ed..a25bdd9 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -99,7 +99,8 @@ static void elevator_put(struct elevator_type *e)
 	module_put(e->elevator_owner);
 }
 
-static struct elevator_type *elevator_get(const char *name, bool try_loading)
+static struct elevator_type *elevator_get(const char *name, bool try_loading,
+					  bool mq_ops)
 {
 	struct elevator_type *e;
 
@@ -113,6 +114,12 @@ static struct elevator_type *elevator_get(const char *name, bool try_loading)
 		e = elevator_find(name);
 	}
 
+	if (e && (e->uses_mq != mq_ops)) {
+		pr_err("ERROR: attempted to choose %s %s I/O scheduler in blk%s",
+		       name, e->uses_mq ? "blk-mq" : "legacy", mq_ops ? "-mq" : "");
+		e = NULL;
+	}
+
 	if (e && !try_module_get(e->elevator_owner))
 		e = NULL;
 
@@ -201,7 +208,7 @@ int elevator_init(struct request_queue *q, char *name)
 	q->boundary_rq = NULL;
 
 	if (name) {
-		e = elevator_get(name, true);
+		e = elevator_get(name, true, q->mq_ops);
 		if (!e)
 			return -EINVAL;
 	}
@@ -212,7 +219,7 @@ int elevator_init(struct request_queue *q, char *name)
 	 * off async and request_module() isn't allowed from async.
 	 */
 	if (!e && *chosen_elevator) {
-		e = elevator_get(chosen_elevator, false);
+		e = elevator_get(chosen_elevator, false, q->mq_ops);
 		if (!e)
 			printk(KERN_ERR "I/O scheduler %s not found\n",
 							chosen_elevator);
@@ -220,17 +227,20 @@ int elevator_init(struct request_queue *q, char *name)
 
 	if (!e) {
 		if (q->mq_ops && q->nr_hw_queues == 1)
-			e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
+			e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false,
+					 q->mq_ops);
 		else if (q->mq_ops)
-			e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
+			e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false,
+					 q->mq_ops);
 		else
-			e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);
+			e = elevator_get(CONFIG_DEFAULT_IOSCHED, false,
+					 q->mq_ops);
 
 		if (!e) {
 			printk(KERN_ERR
 				"Default I/O scheduler not found. " \
 				"Using noop/none.\n");
-			e = elevator_get("noop", false);
+			e = elevator_get("noop", false, q->mq_ops);
 		}
 	}
 
@@ -1051,7 +1061,7 @@ static int __elevator_change(struct request_queue *q, const char *name)
 		return elevator_switch(q, NULL);
 
 	strlcpy(elevator_name, name, sizeof(elevator_name));
-	e = elevator_get(strstrip(elevator_name), true);
+	e = elevator_get(strstrip(elevator_name), true, q->mq_ops);
 	if (!e) {
 		printk(KERN_ERR "elevator: type %s not found\n", elevator_name);
 		return -EINVAL;
-- 
2.10.0

Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice

[Bart Van Assche]

On Mon, 2017-02-13 at 22:01 +0100, Paolo Valente wrote:
> -static struct elevator_type *elevator_get(const char *name, bool try_loading)
> +static struct elevator_type *elevator_get(const char *name, bool try_loading,
> +					  bool mq_ops)

Please choose a better name for that argument, e.q. "mq". To me the name "mq_ops"
means "a pointer to a data structure with operation function pointers".

> +	if (e && (e->uses_mq != mq_ops)) {
> +		pr_err("ERROR: attempted to choose %s %s I/O scheduler in blk%s",
> +		       name, e->uses_mq ? "blk-mq" : "legacy", mq_ops ? "-mq" : "");
> +		e = NULL;
> +	}

How about changing the above into:

+       if (e && e->uses_mq != mq) {
+               pr_err("ERROR: attempt to configure %s as I/O scheduler for a %s queue\n",
+                      name, mq ? "blk-mq" : "legacy");
+               e = NULL;
+       }

Thanks,

Bart.

Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice

[Omar Sandoval]

On Mon, Feb 13, 2017 at 10:01:07PM +0100, Paolo Valente wrote:
> If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq,
> or, viceversa, a blk-mq scheduler is chosen for a device using blk, then
> that scheduler is set and initialized without any check, driving the
> system into an inconsistent state. This commit addresses this issue by
> letting elevator_get fail for these wrong cross choices.
> 
> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
> ---
>  block/elevator.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)

Hey, Paolo,

How exactly are you triggering this? In __elevator_change(), we do check
for mq or not mq:

	if (!e->uses_mq && q->mq_ops) {
		elevator_put(e);
		return -EINVAL;
	}
	if (e->uses_mq && !q->mq_ops) {
		elevator_put(e);
		return -EINVAL;
	}

We don't ever appear to call elevator_init() with a specific scheduler
name, and for the default we switch off of q->mq_ops and use the
defaults from Kconfig:

	if (q->mq_ops && q->nr_hw_queues == 1)
		e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
	else if (q->mq_ops)
		e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
	else
		e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);

	if (!e) {
		printk(KERN_ERR
			"Default I/O scheduler not found. " \
			"Using noop/none.\n");
		e = elevator_get("noop", false);
	}

So I guess this could happen if someone manually changed those Kconfig
options, but I don't see what other case would make this happen, could
you please explain?

Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice

[Jens Axboe]

On 02/13/2017 03:09 PM, Omar Sandoval wrote:
> On Mon, Feb 13, 2017 at 10:01:07PM +0100, Paolo Valente wrote:
>> If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq,
>> or, viceversa, a blk-mq scheduler is chosen for a device using blk, then
>> that scheduler is set and initialized without any check, driving the
>> system into an inconsistent state. This commit addresses this issue by
>> letting elevator_get fail for these wrong cross choices.
>>
>> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
>> ---
>>  block/elevator.c | 26 ++++++++++++++++++--------
>>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> Hey, Paolo,
> 
> How exactly are you triggering this? In __elevator_change(), we do check
> for mq or not mq:
> 
> 	if (!e->uses_mq && q->mq_ops) {
> 		elevator_put(e);
> 		return -EINVAL;
> 	}
> 	if (e->uses_mq && !q->mq_ops) {
> 		elevator_put(e);
> 		return -EINVAL;
> 	}
> 
> We don't ever appear to call elevator_init() with a specific scheduler
> name, and for the default we switch off of q->mq_ops and use the
> defaults from Kconfig:
> 
> 	if (q->mq_ops && q->nr_hw_queues == 1)
> 		e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
> 	else if (q->mq_ops)
> 		e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
> 	else
> 		e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);
> 
> 	if (!e) {
> 		printk(KERN_ERR
> 			"Default I/O scheduler not found. " \
> 			"Using noop/none.\n");
> 		e = elevator_get("noop", false);
> 	}
> 
> So I guess this could happen if someone manually changed those Kconfig
> options, but I don't see what other case would make this happen, could
> you please explain?

Was wondering the same - is it using the 'elevator=' boot parameter?
Didn't look at that path just now, but that's the only one I could
think of. If it is, I'd much prefer only using 'chosen_elevator' for
the non-mq stuff, and the fix should be just that instead.

So instead of:

	if (!e && *chosen_elevator) {

do

	if (!e && !q->mq_ops && && *chosen_elevator) {

-- 
Jens Axboe

Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice

[Jens Axboe]

On 02/13/2017 03:28 PM, Jens Axboe wrote:
> On 02/13/2017 03:09 PM, Omar Sandoval wrote:
>> On Mon, Feb 13, 2017 at 10:01:07PM +0100, Paolo Valente wrote:
>>> If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq,
>>> or, viceversa, a blk-mq scheduler is chosen for a device using blk, then
>>> that scheduler is set and initialized without any check, driving the
>>> system into an inconsistent state. This commit addresses this issue by
>>> letting elevator_get fail for these wrong cross choices.
>>>
>>> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
>>> ---
>>>  block/elevator.c | 26 ++++++++++++++++++--------
>>>  1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> Hey, Paolo,
>>
>> How exactly are you triggering this? In __elevator_change(), we do check
>> for mq or not mq:
>>
>> 	if (!e->uses_mq && q->mq_ops) {
>> 		elevator_put(e);
>> 		return -EINVAL;
>> 	}
>> 	if (e->uses_mq && !q->mq_ops) {
>> 		elevator_put(e);
>> 		return -EINVAL;
>> 	}
>>
>> We don't ever appear to call elevator_init() with a specific scheduler
>> name, and for the default we switch off of q->mq_ops and use the
>> defaults from Kconfig:
>>
>> 	if (q->mq_ops && q->nr_hw_queues == 1)
>> 		e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
>> 	else if (q->mq_ops)
>> 		e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
>> 	else
>> 		e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);
>>
>> 	if (!e) {
>> 		printk(KERN_ERR
>> 			"Default I/O scheduler not found. " \
>> 			"Using noop/none.\n");
>> 		e = elevator_get("noop", false);
>> 	}
>>
>> So I guess this could happen if someone manually changed those Kconfig
>> options, but I don't see what other case would make this happen, could
>> you please explain?
> 
> Was wondering the same - is it using the 'elevator=' boot parameter?
> Didn't look at that path just now, but that's the only one I could
> think of. If it is, I'd much prefer only using 'chosen_elevator' for
> the non-mq stuff, and the fix should be just that instead.
> 
> So instead of:
> 
> 	if (!e && *chosen_elevator) {
> 
> do
> 
> 	if (!e && !q->mq_ops && && *chosen_elevator) {

Confirmed, that's what it seems to be, and here's a real diff of the
above example that works for me:

diff --git a/block/elevator.c b/block/elevator.c
index 27ff1ed5a6fa..699d10f71a2c 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -207,11 +207,12 @@ int elevator_init(struct request_queue *q, char *name)
 	}
 
 	/*
-	 * Use the default elevator specified by config boot param or
-	 * config option.  Don't try to load modules as we could be running
-	 * off async and request_module() isn't allowed from async.
+	 * Use the default elevator specified by config boot param for
+	 * non-mq devices, or by config option. Don't try to load modules
+	 * as we could be running off async and request_module() isn't
+	 * allowed from async.
 	 */
-	if (!e && *chosen_elevator) {
+	if (!e && !q->mq_ops && *chosen_elevator) {
 		e = elevator_get(chosen_elevator, false);
 		if (!e)
 			printk(KERN_ERR "I/O scheduler %s not found\n",

-- 
Jens Axboe

Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice

[Hannes Reinecke]

On 02/13/2017 11:28 PM, Jens Axboe wrote:
> On 02/13/2017 03:09 PM, Omar Sandoval wrote:
>> On Mon, Feb 13, 2017 at 10:01:07PM +0100, Paolo Valente wrote:
>>> If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq,
>>> or, viceversa, a blk-mq scheduler is chosen for a device using blk, then
>>> that scheduler is set and initialized without any check, driving the
>>> system into an inconsistent state. This commit addresses this issue by
>>> letting elevator_get fail for these wrong cross choices.
>>>
>>> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
>>> ---
>>>  block/elevator.c | 26 ++++++++++++++++++--------
>>>  1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> Hey, Paolo,
>>
>> How exactly are you triggering this? In __elevator_change(), we do check
>> for mq or not mq:
>>
>> 	if (!e->uses_mq && q->mq_ops) {
>> 		elevator_put(e);
>> 		return -EINVAL;
>> 	}
>> 	if (e->uses_mq && !q->mq_ops) {
>> 		elevator_put(e);
>> 		return -EINVAL;
>> 	}
>>
>> We don't ever appear to call elevator_init() with a specific scheduler
>> name, and for the default we switch off of q->mq_ops and use the
>> defaults from Kconfig:
>>
>> 	if (q->mq_ops && q->nr_hw_queues == 1)
>> 		e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
>> 	else if (q->mq_ops)
>> 		e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
>> 	else
>> 		e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);
>>
>> 	if (!e) {
>> 		printk(KERN_ERR
>> 			"Default I/O scheduler not found. " \
>> 			"Using noop/none.\n");
>> 		e = elevator_get("noop", false);
>> 	}
>>
>> So I guess this could happen if someone manually changed those Kconfig
>> options, but I don't see what other case would make this happen, could
>> you please explain?
> 
> Was wondering the same - is it using the 'elevator=' boot parameter?
> Didn't look at that path just now, but that's the only one I could
> think of. If it is, I'd much prefer only using 'chosen_elevator' for
> the non-mq stuff, and the fix should be just that instead.
> 
[ .. ]
While we're at the topic:

Can't we use the same names for legacy and mq scheduler?
It's quite an unnecessary complication to have
'noop', 'deadline', and 'cfq' for legacy, but 'none' and 'mq-deadline'
for mq. If we could use 'noop' and 'deadline' for mq, too, the existing
settings or udev rules will continue to work and we wouldn't get any
annoying and pointless warnings here...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice

[Omar Sandoval]

On Tue, Feb 14, 2017 at 07:58:22AM +0100, Hannes Reinecke wrote:
> While we're at the topic:
> 
> Can't we use the same names for legacy and mq scheduler?
> It's quite an unnecessary complication to have
> 'noop', 'deadline', and 'cfq' for legacy, but 'none' and 'mq-deadline'
> for mq. If we could use 'noop' and 'deadline' for mq, too, the existing
> settings or udev rules will continue to work and we wouldn't get any
> annoying and pointless warnings here...

I mentioned this to Jens a little while ago but I didn't feel strongly
enough to push the issue. I also like this idea -- it makes the
transition to blk-mq a little more transparent.

Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice

[Hannes Reinecke]

On 02/14/2017 08:07 AM, Omar Sandoval wrote:
> On Tue, Feb 14, 2017 at 07:58:22AM +0100, Hannes Reinecke wrote:
>> While we're at the topic:
>>
>> Can't we use the same names for legacy and mq scheduler?
>> It's quite an unnecessary complication to have
>> 'noop', 'deadline', and 'cfq' for legacy, but 'none' and 'mq-deadline'
>> for mq. If we could use 'noop' and 'deadline' for mq, too, the existing
>> settings or udev rules will continue to work and we wouldn't get any
>> annoying and pointless warnings here...
> 
> I mentioned this to Jens a little while ago but I didn't feel strongly
> enough to push the issue. I also like this idea -- it makes the
> transition to blk-mq a little more transparent.
> 
And saves us _a lot_ of support cases :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice

[Paolo Valente]

> Il giorno 14 feb 2017, alle ore 00:10, Jens Axboe <axboe@kernel.dk> ha scritto:
> 
> On 02/13/2017 03:28 PM, Jens Axboe wrote:
>> On 02/13/2017 03:09 PM, Omar Sandoval wrote:
>>> On Mon, Feb 13, 2017 at 10:01:07PM +0100, Paolo Valente wrote:
>>>> If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq,
>>>> or, viceversa, a blk-mq scheduler is chosen for a device using blk, then
>>>> that scheduler is set and initialized without any check, driving the
>>>> system into an inconsistent state. This commit addresses this issue by
>>>> letting elevator_get fail for these wrong cross choices.
>>>> 
>>>> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
>>>> ---
>>>> block/elevator.c | 26 ++++++++++++++++++--------
>>>> 1 file changed, 18 insertions(+), 8 deletions(-)
>>> 
>>> Hey, Paolo,
>>> 
>>> How exactly are you triggering this? In __elevator_change(), we do check
>>> for mq or not mq:
>>> 
>>> 	if (!e->uses_mq && q->mq_ops) {
>>> 		elevator_put(e);
>>> 		return -EINVAL;
>>> 	}
>>> 	if (e->uses_mq && !q->mq_ops) {
>>> 		elevator_put(e);
>>> 		return -EINVAL;
>>> 	}
>>> 
>>> We don't ever appear to call elevator_init() with a specific scheduler
>>> name, and for the default we switch off of q->mq_ops and use the
>>> defaults from Kconfig:
>>> 
>>> 	if (q->mq_ops && q->nr_hw_queues == 1)
>>> 		e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
>>> 	else if (q->mq_ops)
>>> 		e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
>>> 	else
>>> 		e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);
>>> 
>>> 	if (!e) {
>>> 		printk(KERN_ERR
>>> 			"Default I/O scheduler not found. " \
>>> 			"Using noop/none.\n");
>>> 		e = elevator_get("noop", false);
>>> 	}
>>> 
>>> So I guess this could happen if someone manually changed those Kconfig
>>> options, but I don't see what other case would make this happen, could
>>> you please explain?
>> 
>> Was wondering the same - is it using the 'elevator=' boot parameter?
>> Didn't look at that path just now, but that's the only one I could
>> think of. If it is, I'd much prefer only using 'chosen_elevator' for
>> the non-mq stuff, and the fix should be just that instead.
>> 
>> So instead of:
>> 
>> 	if (!e && *chosen_elevator) {
>> 
>> do
>> 
>> 	if (!e && !q->mq_ops && && *chosen_elevator) {
> 
> Confirmed, that's what it seems to be, and here's a real diff of the
> above example that works for me:
> 
> diff --git a/block/elevator.c b/block/elevator.c
> index 27ff1ed5a6fa..699d10f71a2c 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -207,11 +207,12 @@ int elevator_init(struct request_queue *q, char *name)
> 	}
> 
> 	/*
> -	 * Use the default elevator specified by config boot param or
> -	 * config option.  Don't try to load modules as we could be running
> -	 * off async and request_module() isn't allowed from async.
> +	 * Use the default elevator specified by config boot param for
> +	 * non-mq devices, or by config option.

I don't fully get this choice: being able to change the default I/O
scheduler through the command line has been rather useful for me,
saving me a lot of recompilations, and such a feature seems widespread
among (at least power) users.  However, mine is of course just an
opinion, and I may be missing the main point also in this case.

Thanks,
Paolo


> Don't try to load modules
> +	 * as we could be running off async and request_module() isn't
> +	 * allowed from async.
> 	 */
> -	if (!e && *chosen_elevator) {
> +	if (!e && !q->mq_ops && *chosen_elevator) {
> 		e = elevator_get(chosen_elevator, false);
> 		if (!e)
> 			printk(KERN_ERR "I/O scheduler %s not found\n",
> 
> -- 
> Jens Axboe

Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice

[Jens Axboe]

On 02/13/2017 11:58 PM, Hannes Reinecke wrote:
> On 02/13/2017 11:28 PM, Jens Axboe wrote:
>> On 02/13/2017 03:09 PM, Omar Sandoval wrote:
>>> On Mon, Feb 13, 2017 at 10:01:07PM +0100, Paolo Valente wrote:
>>>> If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq,
>>>> or, viceversa, a blk-mq scheduler is chosen for a device using blk, then
>>>> that scheduler is set and initialized without any check, driving the
>>>> system into an inconsistent state. This commit addresses this issue by
>>>> letting elevator_get fail for these wrong cross choices.
>>>>
>>>> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
>>>> ---
>>>>  block/elevator.c | 26 ++++++++++++++++++--------
>>>>  1 file changed, 18 insertions(+), 8 deletions(-)
>>>
>>> Hey, Paolo,
>>>
>>> How exactly are you triggering this? In __elevator_change(), we do check
>>> for mq or not mq:
>>>
>>> 	if (!e->uses_mq && q->mq_ops) {
>>> 		elevator_put(e);
>>> 		return -EINVAL;
>>> 	}
>>> 	if (e->uses_mq && !q->mq_ops) {
>>> 		elevator_put(e);
>>> 		return -EINVAL;
>>> 	}
>>>
>>> We don't ever appear to call elevator_init() with a specific scheduler
>>> name, and for the default we switch off of q->mq_ops and use the
>>> defaults from Kconfig:
>>>
>>> 	if (q->mq_ops && q->nr_hw_queues == 1)
>>> 		e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
>>> 	else if (q->mq_ops)
>>> 		e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
>>> 	else
>>> 		e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);
>>>
>>> 	if (!e) {
>>> 		printk(KERN_ERR
>>> 			"Default I/O scheduler not found. " \
>>> 			"Using noop/none.\n");
>>> 		e = elevator_get("noop", false);
>>> 	}
>>>
>>> So I guess this could happen if someone manually changed those Kconfig
>>> options, but I don't see what other case would make this happen, could
>>> you please explain?
>>
>> Was wondering the same - is it using the 'elevator=' boot parameter?
>> Didn't look at that path just now, but that's the only one I could
>> think of. If it is, I'd much prefer only using 'chosen_elevator' for
>> the non-mq stuff, and the fix should be just that instead.
>>
> [ .. ]
> While we're at the topic:
> 
> Can't we use the same names for legacy and mq scheduler?
> It's quite an unnecessary complication to have
> 'noop', 'deadline', and 'cfq' for legacy, but 'none' and 'mq-deadline'
> for mq. If we could use 'noop' and 'deadline' for mq, too, the existing
> settings or udev rules will continue to work and we wouldn't get any
> annoying and pointless warnings here...

I'm fine with potentially renaming mq-deadline to deadline, but I don't
want to mix up none and noop. One is an actual scheduler, the other is
not.

-- 
Jens Axboe

Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice

[Jens Axboe]

On 02/14/2017 01:14 AM, Paolo Valente wrote:
> 
>> Il giorno 14 feb 2017, alle ore 00:10, Jens Axboe <axboe@kernel.dk> ha scritto:
>>
>> On 02/13/2017 03:28 PM, Jens Axboe wrote:
>>> On 02/13/2017 03:09 PM, Omar Sandoval wrote:
>>>> On Mon, Feb 13, 2017 at 10:01:07PM +0100, Paolo Valente wrote:
>>>>> If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq,
>>>>> or, viceversa, a blk-mq scheduler is chosen for a device using blk, then
>>>>> that scheduler is set and initialized without any check, driving the
>>>>> system into an inconsistent state. This commit addresses this issue by
>>>>> letting elevator_get fail for these wrong cross choices.
>>>>>
>>>>> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
>>>>> ---
>>>>> block/elevator.c | 26 ++++++++++++++++++--------
>>>>> 1 file changed, 18 insertions(+), 8 deletions(-)
>>>>
>>>> Hey, Paolo,
>>>>
>>>> How exactly are you triggering this? In __elevator_change(), we do check
>>>> for mq or not mq:
>>>>
>>>> 	if (!e->uses_mq && q->mq_ops) {
>>>> 		elevator_put(e);
>>>> 		return -EINVAL;
>>>> 	}
>>>> 	if (e->uses_mq && !q->mq_ops) {
>>>> 		elevator_put(e);
>>>> 		return -EINVAL;
>>>> 	}
>>>>
>>>> We don't ever appear to call elevator_init() with a specific scheduler
>>>> name, and for the default we switch off of q->mq_ops and use the
>>>> defaults from Kconfig:
>>>>
>>>> 	if (q->mq_ops && q->nr_hw_queues == 1)
>>>> 		e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
>>>> 	else if (q->mq_ops)
>>>> 		e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
>>>> 	else
>>>> 		e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);
>>>>
>>>> 	if (!e) {
>>>> 		printk(KERN_ERR
>>>> 			"Default I/O scheduler not found. " \
>>>> 			"Using noop/none.\n");
>>>> 		e = elevator_get("noop", false);
>>>> 	}
>>>>
>>>> So I guess this could happen if someone manually changed those Kconfig
>>>> options, but I don't see what other case would make this happen, could
>>>> you please explain?
>>>
>>> Was wondering the same - is it using the 'elevator=' boot parameter?
>>> Didn't look at that path just now, but that's the only one I could
>>> think of. If it is, I'd much prefer only using 'chosen_elevator' for
>>> the non-mq stuff, and the fix should be just that instead.
>>>
>>> So instead of:
>>>
>>> 	if (!e && *chosen_elevator) {
>>>
>>> do
>>>
>>> 	if (!e && !q->mq_ops && && *chosen_elevator) {
>>
>> Confirmed, that's what it seems to be, and here's a real diff of the
>> above example that works for me:
>>
>> diff --git a/block/elevator.c b/block/elevator.c
>> index 27ff1ed5a6fa..699d10f71a2c 100644
>> --- a/block/elevator.c
>> +++ b/block/elevator.c
>> @@ -207,11 +207,12 @@ int elevator_init(struct request_queue *q, char *name)
>> 	}
>>
>> 	/*
>> -	 * Use the default elevator specified by config boot param or
>> -	 * config option.  Don't try to load modules as we could be running
>> -	 * off async and request_module() isn't allowed from async.
>> +	 * Use the default elevator specified by config boot param for
>> +	 * non-mq devices, or by config option.
> 
> I don't fully get this choice: being able to change the default I/O
> scheduler through the command line has been rather useful for me,
> saving me a lot of recompilations, and such a feature seems widespread
> among (at least power) users.  However, mine is of course just an
> opinion, and I may be missing the main point also in this case.

The problem with the elevator= boot parameter is that it applies across
everything, which makes very little sense, since it's a per device
setting. In retrospect, it was a mistake to add this parameter, and I
don't want to continue down that path with blk-mq.

Why aren't you just using online switching through sysfs? For normal
users, typically this would be done through udev rules.

-- 
Jens Axboe

Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice

[Paolo Valente]

> Il giorno 14 feb 2017, alle ore 16:16, Jens Axboe <axboe@kernel.dk> ha scritto:
> 
> On 02/14/2017 01:14 AM, Paolo Valente wrote:
>> 
>>> Il giorno 14 feb 2017, alle ore 00:10, Jens Axboe <axboe@kernel.dk> ha scritto:
>>> 
>>> On 02/13/2017 03:28 PM, Jens Axboe wrote:
>>>> On 02/13/2017 03:09 PM, Omar Sandoval wrote:
>>>>> On Mon, Feb 13, 2017 at 10:01:07PM +0100, Paolo Valente wrote:
>>>>>> If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq,
>>>>>> or, viceversa, a blk-mq scheduler is chosen for a device using blk, then
>>>>>> that scheduler is set and initialized without any check, driving the
>>>>>> system into an inconsistent state. This commit addresses this issue by
>>>>>> letting elevator_get fail for these wrong cross choices.
>>>>>> 
>>>>>> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
>>>>>> ---
>>>>>> block/elevator.c | 26 ++++++++++++++++++--------
>>>>>> 1 file changed, 18 insertions(+), 8 deletions(-)
>>>>> 
>>>>> Hey, Paolo,
>>>>> 
>>>>> How exactly are you triggering this? In __elevator_change(), we do check
>>>>> for mq or not mq:
>>>>> 
>>>>> 	if (!e->uses_mq && q->mq_ops) {
>>>>> 		elevator_put(e);
>>>>> 		return -EINVAL;
>>>>> 	}
>>>>> 	if (e->uses_mq && !q->mq_ops) {
>>>>> 		elevator_put(e);
>>>>> 		return -EINVAL;
>>>>> 	}
>>>>> 
>>>>> We don't ever appear to call elevator_init() with a specific scheduler
>>>>> name, and for the default we switch off of q->mq_ops and use the
>>>>> defaults from Kconfig:
>>>>> 
>>>>> 	if (q->mq_ops && q->nr_hw_queues == 1)
>>>>> 		e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
>>>>> 	else if (q->mq_ops)
>>>>> 		e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
>>>>> 	else
>>>>> 		e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);
>>>>> 
>>>>> 	if (!e) {
>>>>> 		printk(KERN_ERR
>>>>> 			"Default I/O scheduler not found. " \
>>>>> 			"Using noop/none.\n");
>>>>> 		e = elevator_get("noop", false);
>>>>> 	}
>>>>> 
>>>>> So I guess this could happen if someone manually changed those Kconfig
>>>>> options, but I don't see what other case would make this happen, could
>>>>> you please explain?
>>>> 
>>>> Was wondering the same - is it using the 'elevator=' boot parameter?
>>>> Didn't look at that path just now, but that's the only one I could
>>>> think of. If it is, I'd much prefer only using 'chosen_elevator' for
>>>> the non-mq stuff, and the fix should be just that instead.
>>>> 
>>>> So instead of:
>>>> 
>>>> 	if (!e && *chosen_elevator) {
>>>> 
>>>> do
>>>> 
>>>> 	if (!e && !q->mq_ops && && *chosen_elevator) {
>>> 
>>> Confirmed, that's what it seems to be, and here's a real diff of the
>>> above example that works for me:
>>> 
>>> diff --git a/block/elevator.c b/block/elevator.c
>>> index 27ff1ed5a6fa..699d10f71a2c 100644
>>> --- a/block/elevator.c
>>> +++ b/block/elevator.c
>>> @@ -207,11 +207,12 @@ int elevator_init(struct request_queue *q, char *name)
>>> 	}
>>> 
>>> 	/*
>>> -	 * Use the default elevator specified by config boot param or
>>> -	 * config option.  Don't try to load modules as we could be running
>>> -	 * off async and request_module() isn't allowed from async.
>>> +	 * Use the default elevator specified by config boot param for
>>> +	 * non-mq devices, or by config option.
>> 
>> I don't fully get this choice: being able to change the default I/O
>> scheduler through the command line has been rather useful for me,
>> saving me a lot of recompilations, and such a feature seems widespread
>> among (at least power) users.  However, mine is of course just an
>> opinion, and I may be missing the main point also in this case.
> 
> The problem with the elevator= boot parameter is that it applies across
> everything, which makes very little sense, since it's a per device
> setting. In retrospect, it was a mistake to add this parameter, and I
> don't want to continue down that path with blk-mq.
> 

ok, thanks

> Why aren't you just using online switching through syses?

To change the scheduler from the very beginning at boot.  Which maybe
can be done through udev rules as well, I'm just too ignorant.

Thanks,
Paolo

> For normal
> users, typically this would be done through udev rules.
> 
> -- 
> Jens Axboe