[PATCH 0/3] mq vs non-mq scheduler tweaks

[PATCH 0/3] mq vs non-mq scheduler tweaks

[Jens Axboe]

Small series that streamlines this a little bit, and also introduces
name aliases since that's now easy to do with the mq vs non-mq
explicit scheduler lookups.

[PATCH 1/3] elevator: lookup mq vs non-mq elevators

[Jens Axboe]

If an IO scheduler is selected via elevator= and it doesn't match
the driver in question wrt blk-mq support, then we fail to boot.

The elevator= parameter is deprecated and only supported for
non-mq devices. Augment the elevator lookup API so that we
pass in if we're looking for an mq capable scheduler or not,
so that we only ever return a valid type for the queue in
question.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=196695
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/elevator.c | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index 7ae50eb2732b..b70e69f795db 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -83,12 +83,15 @@ bool elv_bio_merge_ok(struct request *rq, struct bio *bio)
 }
 EXPORT_SYMBOL(elv_bio_merge_ok);
 
-static struct elevator_type *elevator_find(const char *name)
+/*
+ * Return scheduler with name 'name' and with matching 'mq capability
+ */
+static struct elevator_type *elevator_find(const char *name, bool mq)
 {
 	struct elevator_type *e;
 
 	list_for_each_entry(e, &elv_list, list) {
-		if (!strcmp(e->elevator_name, name))
+		if (!strcmp(e->elevator_name, name) && !(mq ^ e->uses_mq))
 			return e;
 	}
 
@@ -100,25 +103,25 @@ 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(struct request_queue *q,
+					  const char *name, bool try_loading)
 {
 	struct elevator_type *e;
 
 	spin_lock(&elv_list_lock);
 
-	e = elevator_find(name);
+	e = elevator_find(name, q->mq_ops != NULL);
 	if (!e && try_loading) {
 		spin_unlock(&elv_list_lock);
 		request_module("%s-iosched", name);
 		spin_lock(&elv_list_lock);
-		e = elevator_find(name);
+		e = elevator_find(name, q->mq_ops != NULL);
 	}
 
 	if (e && !try_module_get(e->elevator_owner))
 		e = NULL;
 
 	spin_unlock(&elv_list_lock);
-
 	return e;
 }
 
@@ -144,8 +147,12 @@ void __init load_default_elevator_module(void)
 	if (!chosen_elevator[0])
 		return;
 
+	/*
+	 * Boot parameter is deprecated, we haven't supported that for MQ.
+	 * Only look for non-mq schedulers from here.
+	 */
 	spin_lock(&elv_list_lock);
-	e = elevator_find(chosen_elevator);
+	e = elevator_find(chosen_elevator, false);
 	spin_unlock(&elv_list_lock);
 
 	if (!e)
@@ -202,7 +209,7 @@ int elevator_init(struct request_queue *q, char *name)
 	q->boundary_rq = NULL;
 
 	if (name) {
-		e = elevator_get(name, true);
+		e = elevator_get(q, name, true);
 		if (!e)
 			return -EINVAL;
 	}
@@ -214,7 +221,7 @@ int elevator_init(struct request_queue *q, char *name)
 	 * allowed from async.
 	 */
 	if (!e && !q->mq_ops && *chosen_elevator) {
-		e = elevator_get(chosen_elevator, false);
+		e = elevator_get(q, chosen_elevator, false);
 		if (!e)
 			printk(KERN_ERR "I/O scheduler %s not found\n",
 							chosen_elevator);
@@ -229,17 +236,17 @@ int elevator_init(struct request_queue *q, char *name)
 		 */
 		if (q->mq_ops) {
 			if (q->nr_hw_queues == 1)
-				e = elevator_get("mq-deadline", false);
+				e = elevator_get(q, "mq-deadline", false);
 			if (!e)
 				return 0;
 		} else
-			e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);
+			e = elevator_get(q, CONFIG_DEFAULT_IOSCHED, false);
 
 		if (!e) {
 			printk(KERN_ERR
 				"Default I/O scheduler not found. " \
 				"Using noop.\n");
-			e = elevator_get("noop", false);
+			e = elevator_get(q, "noop", false);
 		}
 	}
 
@@ -905,7 +912,7 @@ int elv_register(struct elevator_type *e)
 
 	/* register, don't allow duplicate names */
 	spin_lock(&elv_list_lock);
-	if (elevator_find(e->elevator_name)) {
+	if (elevator_find(e->elevator_name, e->uses_mq)) {
 		spin_unlock(&elv_list_lock);
 		if (e->icq_cache)
 			kmem_cache_destroy(e->icq_cache);
@@ -1066,7 +1073,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(q, strstrip(elevator_name), true);
 	if (!e)
 		return -EINVAL;
 
@@ -1076,15 +1083,6 @@ static int __elevator_change(struct request_queue *q, const char *name)
 		return 0;
 	}
 
-	if (!e->uses_mq && q->mq_ops) {
-		elevator_put(e);
-		return -EINVAL;
-	}
-	if (e->uses_mq && !q->mq_ops) {
-		elevator_put(e);
-		return -EINVAL;
-	}
-
 	return elevator_switch(q, e);
 }
 
-- 
2.7.4

Re: [PATCH 1/3] elevator: lookup mq vs non-mq elevators

[Omar Sandoval]

On Wed, Oct 25, 2017 at 12:04:45PM -0600, Jens Axboe wrote:
> If an IO scheduler is selected via elevator= and it doesn't match
> the driver in question wrt blk-mq support, then we fail to boot.
> 
> The elevator= parameter is deprecated and only supported for
> non-mq devices. Augment the elevator lookup API so that we
> pass in if we're looking for an mq capable scheduler or not,
> so that we only ever return a valid type for the queue in
> question.
> 
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=196695
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  block/elevator.c | 44 +++++++++++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/block/elevator.c b/block/elevator.c
> index 7ae50eb2732b..b70e69f795db 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -83,12 +83,15 @@ bool elv_bio_merge_ok(struct request *rq, struct bio *bio)
>  }
>  EXPORT_SYMBOL(elv_bio_merge_ok);
>  
> -static struct elevator_type *elevator_find(const char *name)
> +/*
> + * Return scheduler with name 'name' and with matching 'mq capability
> + */
> +static struct elevator_type *elevator_find(const char *name, bool mq)
>  {
>  	struct elevator_type *e;
>  
>  	list_for_each_entry(e, &elv_list, list) {
> -		if (!strcmp(e->elevator_name, name))
> +		if (!strcmp(e->elevator_name, name) && !(mq ^ e->uses_mq))


How about just (mq == e->uses_mq)?

Besides that, Reviewed-by: Omar Sandoval <osandov@fb.com>

>  			return e;
>  	}
>  

Re: [PATCH 1/3] elevator: lookup mq vs non-mq elevators

[Jens Axboe]

On 10/25/2017 11:27 AM, Omar Sandoval wrote:
> On Wed, Oct 25, 2017 at 12:04:45PM -0600, Jens Axboe wrote:
>> If an IO scheduler is selected via elevator= and it doesn't match
>> the driver in question wrt blk-mq support, then we fail to boot.
>>
>> The elevator= parameter is deprecated and only supported for
>> non-mq devices. Augment the elevator lookup API so that we
>> pass in if we're looking for an mq capable scheduler or not,
>> so that we only ever return a valid type for the queue in
>> question.
>>
>> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=196695
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  block/elevator.c | 44 +++++++++++++++++++++-----------------------
>>  1 file changed, 21 insertions(+), 23 deletions(-)
>>
>> diff --git a/block/elevator.c b/block/elevator.c
>> index 7ae50eb2732b..b70e69f795db 100644
>> --- a/block/elevator.c
>> +++ b/block/elevator.c
>> @@ -83,12 +83,15 @@ bool elv_bio_merge_ok(struct request *rq, struct bio *bio)
>>  }
>>  EXPORT_SYMBOL(elv_bio_merge_ok);
>>  
>> -static struct elevator_type *elevator_find(const char *name)
>> +/*
>> + * Return scheduler with name 'name' and with matching 'mq capability
>> + */
>> +static struct elevator_type *elevator_find(const char *name, bool mq)
>>  {
>>  	struct elevator_type *e;
>>  
>>  	list_for_each_entry(e, &elv_list, list) {
>> -		if (!strcmp(e->elevator_name, name))
>> +		if (!strcmp(e->elevator_name, name) && !(mq ^ e->uses_mq))
> 
> 
> How about just (mq == e->uses_mq)?

That is simpler/cleaner, I'll make that adjustment. Not sure where the XOR
temptation came from.

-- 
Jens Axboe

[PATCH 2/3] elevator: allow name aliases

[Jens Axboe]

Since we now lookup elevator types with the appropriate multiqueue
capability, allow schedulers to register with an alias alongside
the real name. This is in preparation for allowing 'mq-deadline'
to register an alias of 'deadline' as well.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/elevator.c         | 22 ++++++++++++++++------
 include/linux/elevator.h |  1 +
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index b70e69f795db..dc0432e12ecf 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -83,6 +83,16 @@ bool elv_bio_merge_ok(struct request *rq, struct bio *bio)
 }
 EXPORT_SYMBOL(elv_bio_merge_ok);
 
+static bool elevator_match(const struct elevator_type *e, const char *name)
+{
+	if (!strcmp(e->elevator_name, name))
+		return true;
+	if (e->elevator_alias && !strcmp(e->elevator_alias, name))
+		return true;
+
+	return false;
+}
+
 /*
  * Return scheduler with name 'name' and with matching 'mq capability
  */
@@ -91,7 +101,7 @@ static struct elevator_type *elevator_find(const char *name, bool mq)
 	struct elevator_type *e;
 
 	list_for_each_entry(e, &elv_list, list) {
-		if (!strcmp(e->elevator_name, name) && !(mq ^ e->uses_mq))
+		if (elevator_match(e, name) && !(mq ^ e->uses_mq))
 			return e;
 	}
 
@@ -922,9 +932,9 @@ int elv_register(struct elevator_type *e)
 	spin_unlock(&elv_list_lock);
 
 	/* print pretty message */
-	if (!strcmp(e->elevator_name, chosen_elevator) ||
+	if (elevator_match(e, chosen_elevator) ||
 			(!*chosen_elevator &&
-			 !strcmp(e->elevator_name, CONFIG_DEFAULT_IOSCHED)))
+			 elevator_match(e, CONFIG_DEFAULT_IOSCHED)))
 				def = " (default)";
 
 	printk(KERN_INFO "io scheduler %s registered%s\n", e->elevator_name,
@@ -1077,8 +1087,7 @@ static int __elevator_change(struct request_queue *q, const char *name)
 	if (!e)
 		return -EINVAL;
 
-	if (q->elevator &&
-	    !strcmp(elevator_name, q->elevator->type->elevator_name)) {
+	if (q->elevator && elevator_match(q->elevator->type, elevator_name)) {
 		elevator_put(e);
 		return 0;
 	}
@@ -1126,7 +1135,8 @@ ssize_t elv_iosched_show(struct request_queue *q, char *name)
 
 	spin_lock(&elv_list_lock);
 	list_for_each_entry(__e, &elv_list, list) {
-		if (elv && !strcmp(elv->elevator_name, __e->elevator_name)) {
+		if (elv && elevator_match(elv, __e->elevator_name) &&
+		    !(__e->uses_mq ^ (q->mq_ops != NULL))) {
 			len += sprintf(name+len, "[%s] ", elv->elevator_name);
 			continue;
 		}
diff --git a/include/linux/elevator.h b/include/linux/elevator.h
index 5bc8f8682a3e..6df8b14f1f6a 100644
--- a/include/linux/elevator.h
+++ b/include/linux/elevator.h
@@ -144,6 +144,7 @@ struct elevator_type
 	size_t icq_align;	/* ditto */
 	struct elv_fs_entry *elevator_attrs;
 	char elevator_name[ELV_NAME_MAX];
+	const char *elevator_alias;
 	struct module *elevator_owner;
 	bool uses_mq;
 #ifdef CONFIG_BLK_DEBUG_FS
-- 
2.7.4

Re: [PATCH 2/3] elevator: allow name aliases

[Omar Sandoval]

On Wed, Oct 25, 2017 at 12:04:46PM -0600, Jens Axboe wrote:
> Since we now lookup elevator types with the appropriate multiqueue
> capability, allow schedulers to register with an alias alongside
> the real name. This is in preparation for allowing 'mq-deadline'
> to register an alias of 'deadline' as well.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  block/elevator.c         | 22 ++++++++++++++++------
>  include/linux/elevator.h |  1 +
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/block/elevator.c b/block/elevator.c
> index b70e69f795db..dc0432e12ecf 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -83,6 +83,16 @@ bool elv_bio_merge_ok(struct request *rq, struct bio *bio)
>  }
>  EXPORT_SYMBOL(elv_bio_merge_ok);
>  
> +static bool elevator_match(const struct elevator_type *e, const char *name)
> +{
> +	if (!strcmp(e->elevator_name, name))
> +		return true;
> +	if (e->elevator_alias && !strcmp(e->elevator_alias, name))
> +		return true;
> +
> +	return false;
> +}
> +
>  /*
>   * Return scheduler with name 'name' and with matching 'mq capability
>   */
> @@ -91,7 +101,7 @@ static struct elevator_type *elevator_find(const char *name, bool mq)
>  	struct elevator_type *e;
>  
>  	list_for_each_entry(e, &elv_list, list) {
> -		if (!strcmp(e->elevator_name, name) && !(mq ^ e->uses_mq))
> +		if (elevator_match(e, name) && !(mq ^ e->uses_mq))
>  			return e;
>  	}
>  
> @@ -922,9 +932,9 @@ int elv_register(struct elevator_type *e)
>  	spin_unlock(&elv_list_lock);
>  
>  	/* print pretty message */
> -	if (!strcmp(e->elevator_name, chosen_elevator) ||
> +	if (elevator_match(e, chosen_elevator) ||
>  			(!*chosen_elevator &&
> -			 !strcmp(e->elevator_name, CONFIG_DEFAULT_IOSCHED)))
> +			 elevator_match(e, CONFIG_DEFAULT_IOSCHED)))
>  				def = " (default)";
>  
>  	printk(KERN_INFO "io scheduler %s registered%s\n", e->elevator_name,
> @@ -1077,8 +1087,7 @@ static int __elevator_change(struct request_queue *q, const char *name)
>  	if (!e)
>  		return -EINVAL;
>  
> -	if (q->elevator &&
> -	    !strcmp(elevator_name, q->elevator->type->elevator_name)) {
> +	if (q->elevator && elevator_match(q->elevator->type, elevator_name)) {
>  		elevator_put(e);
>  		return 0;
>  	}
> @@ -1126,7 +1135,8 @@ ssize_t elv_iosched_show(struct request_queue *q, char *name)
>  
>  	spin_lock(&elv_list_lock);
>  	list_for_each_entry(__e, &elv_list, list) {
> -		if (elv && !strcmp(elv->elevator_name, __e->elevator_name)) {
> +		if (elv && elevator_match(elv, __e->elevator_name) &&
> +		    !(__e->uses_mq ^ (q->mq_ops != NULL))) {

Same thing here, (__e->uses_mq == (q->mq_ops != NULL)). Still confusing
but less so.

Otherwise Reviewed-by: Omar Sandoval <osandov@fb.com>

Re: [PATCH 2/3] elevator: allow name aliases

[Jens Axboe]

On 10/25/2017 11:32 AM, Omar Sandoval wrote:
> On Wed, Oct 25, 2017 at 12:04:46PM -0600, Jens Axboe wrote:
>> Since we now lookup elevator types with the appropriate multiqueue
>> capability, allow schedulers to register with an alias alongside
>> the real name. This is in preparation for allowing 'mq-deadline'
>> to register an alias of 'deadline' as well.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  block/elevator.c         | 22 ++++++++++++++++------
>>  include/linux/elevator.h |  1 +
>>  2 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/elevator.c b/block/elevator.c
>> index b70e69f795db..dc0432e12ecf 100644
>> --- a/block/elevator.c
>> +++ b/block/elevator.c
>> @@ -83,6 +83,16 @@ bool elv_bio_merge_ok(struct request *rq, struct bio *bio)
>>  }
>>  EXPORT_SYMBOL(elv_bio_merge_ok);
>>  
>> +static bool elevator_match(const struct elevator_type *e, const char *name)
>> +{
>> +	if (!strcmp(e->elevator_name, name))
>> +		return true;
>> +	if (e->elevator_alias && !strcmp(e->elevator_alias, name))
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>>  /*
>>   * Return scheduler with name 'name' and with matching 'mq capability
>>   */
>> @@ -91,7 +101,7 @@ static struct elevator_type *elevator_find(const char *name, bool mq)
>>  	struct elevator_type *e;
>>  
>>  	list_for_each_entry(e, &elv_list, list) {
>> -		if (!strcmp(e->elevator_name, name) && !(mq ^ e->uses_mq))
>> +		if (elevator_match(e, name) && !(mq ^ e->uses_mq))
>>  			return e;
>>  	}
>>  
>> @@ -922,9 +932,9 @@ int elv_register(struct elevator_type *e)
>>  	spin_unlock(&elv_list_lock);
>>  
>>  	/* print pretty message */
>> -	if (!strcmp(e->elevator_name, chosen_elevator) ||
>> +	if (elevator_match(e, chosen_elevator) ||
>>  			(!*chosen_elevator &&
>> -			 !strcmp(e->elevator_name, CONFIG_DEFAULT_IOSCHED)))
>> +			 elevator_match(e, CONFIG_DEFAULT_IOSCHED)))
>>  				def = " (default)";
>>  
>>  	printk(KERN_INFO "io scheduler %s registered%s\n", e->elevator_name,
>> @@ -1077,8 +1087,7 @@ static int __elevator_change(struct request_queue *q, const char *name)
>>  	if (!e)
>>  		return -EINVAL;
>>  
>> -	if (q->elevator &&
>> -	    !strcmp(elevator_name, q->elevator->type->elevator_name)) {
>> +	if (q->elevator && elevator_match(q->elevator->type, elevator_name)) {
>>  		elevator_put(e);
>>  		return 0;
>>  	}
>> @@ -1126,7 +1135,8 @@ ssize_t elv_iosched_show(struct request_queue *q, char *name)
>>  
>>  	spin_lock(&elv_list_lock);
>>  	list_for_each_entry(__e, &elv_list, list) {
>> -		if (elv && !strcmp(elv->elevator_name, __e->elevator_name)) {
>> +		if (elv && elevator_match(elv, __e->elevator_name) &&
>> +		    !(__e->uses_mq ^ (q->mq_ops != NULL))) {
> 
> Same thing here, (__e->uses_mq == (q->mq_ops != NULL)). Still confusing
> but less so.

I'll just add a

bool uses_mq = q->mq_ops != NULL;

to make it clearer.

-- 
Jens Axboe

[PATCH 3/3] mq-deadline: add 'deadline' as a name alias

[Jens Axboe]

The scheduler framework now supports looking up the appropriate
scheduler with the {name,mq} tupple. We can register mq-deadline
with the alias of 'deadline', so that switching to 'deadline'
will do the right thing based on the type of driver attached to
it.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/mq-deadline.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/mq-deadline.c b/block/mq-deadline.c
index a1cad4331edd..0179e484ec98 100644
--- a/block/mq-deadline.c
+++ b/block/mq-deadline.c
@@ -657,6 +657,7 @@ static struct elevator_type mq_deadline = {
 #endif
 	.elevator_attrs = deadline_attrs,
 	.elevator_name = "mq-deadline",
+	.elevator_alias = "deadline",
 	.elevator_owner = THIS_MODULE,
 };
 MODULE_ALIAS("mq-deadline-iosched");
-- 
2.7.4

Re: [PATCH 3/3] mq-deadline: add 'deadline' as a name alias

[Omar Sandoval]

On Wed, Oct 25, 2017 at 12:04:47PM -0600, Jens Axboe wrote:
> The scheduler framework now supports looking up the appropriate
> scheduler with the {name,mq} tupple. We can register mq-deadline
> with the alias of 'deadline', so that switching to 'deadline'
> will do the right thing based on the type of driver attached to
> it.

Reviewed-by: Omar Sandoval <osandov@fb.com>

A blktest for this would be nice.

> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  block/mq-deadline.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
> index a1cad4331edd..0179e484ec98 100644
> --- a/block/mq-deadline.c
> +++ b/block/mq-deadline.c
> @@ -657,6 +657,7 @@ static struct elevator_type mq_deadline = {
>  #endif
>  	.elevator_attrs = deadline_attrs,
>  	.elevator_name = "mq-deadline",
> +	.elevator_alias = "deadline",
>  	.elevator_owner = THIS_MODULE,
>  };
>  MODULE_ALIAS("mq-deadline-iosched");
> -- 
> 2.7.4
> 

Re: [PATCH 3/3] mq-deadline: add 'deadline' as a name alias

[Jens Axboe]

On 10/25/2017 11:34 AM, Omar Sandoval wrote:
> On Wed, Oct 25, 2017 at 12:04:47PM -0600, Jens Axboe wrote:
>> The scheduler framework now supports looking up the appropriate
>> scheduler with the {name,mq} tupple. We can register mq-deadline
>> with the alias of 'deadline', so that switching to 'deadline'
>> will do the right thing based on the type of driver attached to
>> it.
> 
> Reviewed-by: Omar Sandoval <osandov@fb.com>
> 
> A blktest for this would be nice.

Not really critical imho, as it's not fixing a "real" issue in
terms of an oops, hang, or similar.

-- 
Jens Axboe