misc elevator code cleanups

misc elevator code cleanups

[Christoph Hellwig]

Hi Jens,

this series has a bunch of random elevator cleanups.

Diffstat:
 blk-mq-sched.c |    7 --
 blk-mq.c       |    2 
 blk.h          |    1 
 elevator.c     |  175 +++++++++++++++++++++++----------------------------------
 4 files changed, 73 insertions(+), 112 deletions(-)

[PATCH 1/7] block: drop the duplicate check in elv_register

[Christoph Hellwig]

We have less than a handful of elevators, and if someone adds a duplicate
one it simply will never be found but other be harmless.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/elevator.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index d26aa787e29f0..ef9af17293ffb 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -545,13 +545,7 @@ int elv_register(struct elevator_type *e)
 			return -ENOMEM;
 	}
 
-	/* register, don't allow duplicate names */
 	spin_lock(&elv_list_lock);
-	if (elevator_find(e->elevator_name, 0)) {
-		spin_unlock(&elv_list_lock);
-		kmem_cache_destroy(e->icq_cache);
-		return -EBUSY;
-	}
 	list_add_tail(&e->list, &elv_list);
 	spin_unlock(&elv_list_lock);
 
-- 
2.30.2

Re: [PATCH 1/7] block: drop the duplicate check in elv_register

[Jens Axboe]

On 10/30/22 4:07 AM, Christoph Hellwig wrote:
> We have less than a handful of elevators, and if someone adds a duplicate
> one it simply will never be found but other be harmless.

That isn't quite parseable...

> diff --git a/block/elevator.c b/block/elevator.c
> index d26aa787e29f0..ef9af17293ffb 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -545,13 +545,7 @@ int elv_register(struct elevator_type *e)
>  			return -ENOMEM;
>  	}
>  
> -	/* register, don't allow duplicate names */
>  	spin_lock(&elv_list_lock);
> -	if (elevator_find(e->elevator_name, 0)) {
> -		spin_unlock(&elv_list_lock);
> -		kmem_cache_destroy(e->icq_cache);
> -		return -EBUSY;
> -	}
>  	list_add_tail(&e->list, &elv_list);
>  	spin_unlock(&elv_list_lock);

What's the idea behind this? Yes it'll be harmless and list ordering
will dictate which one will be found, leaving the other(s) dead, but why
not catch this upfront? I agree likelihood of this ever happening to be
tiny, but seems like a good idea to catch and return BUSY for this case.

-- 
Jens Axboe

Re: [PATCH 1/7] block: drop the duplicate check in elv_register

[Christoph Hellwig]

On Mon, Oct 31, 2022 at 07:50:02AM -0600, Jens Axboe wrote:
> >  	list_add_tail(&e->list, &elv_list);
> >  	spin_unlock(&elv_list_lock);
> 
> What's the idea behind this? Yes it'll be harmless and list ordering
> will dictate which one will be found, leaving the other(s) dead, but why
> not catch this upfront? I agree likelihood of this ever happening to be
> tiny, but seems like a good idea to catch and return BUSY for this case.

Because it's just not very useful code bloat here that I stumbled upon.
But I can just drop it if you prefer.

[PATCH 2/7] block: cleanup elevator_get

[Christoph Hellwig]

Do the request_module and repeated lookup in the only caller that cares,
pick a saner name that explains where are actually doing a lookup and
use a sane calling conventions that passes the queue first.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/elevator.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index ef9af17293ffb..d0e48839f6764 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -132,24 +132,15 @@ static struct elevator_type *elevator_find(const char *name,
 	return NULL;
 }
 
-static struct elevator_type *elevator_get(struct request_queue *q,
-					  const char *name, bool try_loading)
+static struct elevator_type *elevator_find_get(struct request_queue *q,
+		const char *name)
 {
 	struct elevator_type *e;
 
 	spin_lock(&elv_list_lock);
-
 	e = elevator_find(name, q->required_elevator_features);
-	if (!e && try_loading) {
-		spin_unlock(&elv_list_lock);
-		request_module("%s-iosched", name);
-		spin_lock(&elv_list_lock);
-		e = elevator_find(name, q->required_elevator_features);
-	}
-
 	if (e && !elevator_tryget(e))
 		e = NULL;
-
 	spin_unlock(&elv_list_lock);
 	return e;
 }
@@ -628,7 +619,7 @@ static struct elevator_type *elevator_get_default(struct request_queue *q)
 	    !blk_mq_is_shared_tags(q->tag_set->flags))
 		return NULL;
 
-	return elevator_get(q, "mq-deadline", false);
+	return elevator_find_get(q, "mq-deadline");
 }
 
 /*
@@ -751,9 +742,13 @@ static int elevator_change(struct request_queue *q, const char *elevator_name)
 	if (q->elevator && elevator_match(q->elevator->type, elevator_name, 0))
 		return 0;
 
-	e = elevator_get(q, elevator_name, true);
-	if (!e)
-		return -EINVAL;
+	e = elevator_find_get(q, elevator_name);
+	if (!e) {
+		request_module("%s-iosched", elevator_name);
+		e = elevator_find_get(q, elevator_name);
+		if (!e)
+			return -EINVAL;
+	}
 	ret = elevator_switch(q, e);
 	elevator_put(e);
 	return ret;
-- 
2.30.2

[PATCH 3/7] block: exit elv_iosched_show early when I/O schedulers are not supported

[Christoph Hellwig]

If the tag_set has BLK_MQ_F_NO_SCHED flag set we will never show any
scheduler, so exit early.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/elevator.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index d0e48839f6764..92096e5aabd36 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -777,7 +777,7 @@ ssize_t elv_iosched_show(struct request_queue *q, char *name)
 	struct elevator_type *__e;
 	int len = 0;
 
-	if (!queue_is_mq(q))
+	if (!elv_support_iosched(q))
 		return sprintf(name, "none\n");
 
 	if (!q->elevator)
@@ -791,8 +791,7 @@ ssize_t elv_iosched_show(struct request_queue *q, char *name)
 			len += sprintf(name+len, "[%s] ", elv->elevator_name);
 			continue;
 		}
-		if (elv_support_iosched(q) &&
-		    elevator_match(__e, __e->elevator_name,
+		if (elevator_match(__e, __e->elevator_name,
 				   q->required_elevator_features))
 			len += sprintf(name+len, "%s ", __e->elevator_name);
 	}
-- 
2.30.2

[PATCH 4/7] block: cleanup the variable naming in elv_iosched_store

[Christoph Hellwig]

Use eq for the elevator_queue as done elsewhere.  This frees e to be used
for the loop iterator instead of the odd __ prefix.  In addition rename
elv to cur to make it more clear it is the currently selected elevator.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/elevator.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index 92096e5aabd36..4fc0d2f539295 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -772,9 +772,8 @@ ssize_t elv_iosched_store(struct request_queue *q, const char *buf,
 
 ssize_t elv_iosched_show(struct request_queue *q, char *name)
 {
-	struct elevator_queue *e = q->elevator;
-	struct elevator_type *elv = NULL;
-	struct elevator_type *__e;
+	struct elevator_queue *eq = q->elevator;
+	struct elevator_type *cur = NULL, *e;
 	int len = 0;
 
 	if (!elv_support_iosched(q))
@@ -783,17 +782,17 @@ ssize_t elv_iosched_show(struct request_queue *q, char *name)
 	if (!q->elevator)
 		len += sprintf(name+len, "[none] ");
 	else
-		elv = e->type;
+		cur = eq->type;
 
 	spin_lock(&elv_list_lock);
-	list_for_each_entry(__e, &elv_list, list) {
-		if (elv && elevator_match(elv, __e->elevator_name, 0)) {
-			len += sprintf(name+len, "[%s] ", elv->elevator_name);
+	list_for_each_entry(e, &elv_list, list) {
+		if (cur && elevator_match(cur, e->elevator_name, 0)) {
+			len += sprintf(name+len, "[%s] ", cur->elevator_name);
 			continue;
 		}
-		if (elevator_match(__e, __e->elevator_name,
+		if (elevator_match(e, e->elevator_name,
 				   q->required_elevator_features))
-			len += sprintf(name+len, "%s ", __e->elevator_name);
+			len += sprintf(name+len, "%s ", e->elevator_name);
 	}
 	spin_unlock(&elv_list_lock);
 
-- 
2.30.2

[PATCH 5/7] block: simplify the check for the current elevator in elv_iosched_show

[Christoph Hellwig]

Just compare the pointers instead of using the string based
elevator_match.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/elevator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/elevator.c b/block/elevator.c
index 4fc0d2f539295..77c16c5ef04ff 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -786,7 +786,7 @@ 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 (cur && elevator_match(cur, e->elevator_name, 0)) {
+		if (e == cur) {
 			len += sprintf(name+len, "[%s] ", cur->elevator_name);
 			continue;
 		}
-- 
2.30.2

[PATCH 6/7] block: don't check for required features in elevator_match

[Christoph Hellwig]

Checking for the required features in the callers simplifies the code
quite a bit, so do that.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/elevator.c | 49 +++++++++++++++---------------------------------
 1 file changed, 15 insertions(+), 34 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index 77c16c5ef04ff..4042e524333e0 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -83,10 +83,11 @@ bool elv_bio_merge_ok(struct request *rq, struct bio *bio)
 }
 EXPORT_SYMBOL(elv_bio_merge_ok);
 
-static inline bool elv_support_features(unsigned int elv_features,
-					unsigned int required_features)
+static inline bool elv_support_features(struct request_queue *q,
+		const struct elevator_type *e)
 {
-	return (required_features & elv_features) == required_features;
+	return (q->required_elevator_features & e->elevator_features) ==
+		q->required_elevator_features;
 }
 
 /**
@@ -98,37 +99,19 @@ static inline bool elv_support_features(unsigned int elv_features,
  * Return true if the elevator @e name matches @name and if @e provides all
  * the features specified by @required_features.
  */
-static bool elevator_match(const struct elevator_type *e, const char *name,
-			   unsigned int required_features)
+static bool elevator_match(const struct elevator_type *e, const char *name)
 {
-	if (!elv_support_features(e->elevator_features, required_features))
-		return false;
-	if (!strcmp(e->elevator_name, name))
-		return true;
-	if (e->elevator_alias && !strcmp(e->elevator_alias, name))
-		return true;
-
-	return false;
+	return !strcmp(e->elevator_name, name) ||
+		(e->elevator_alias && !strcmp(e->elevator_alias, name));
 }
 
-/**
- * elevator_find - Find an elevator
- * @name: Name of the elevator to find
- * @required_features: Features that the elevator must provide
- *
- * Return the first registered scheduler with name @name and supporting the
- * features @required_features and NULL otherwise.
- */
-static struct elevator_type *elevator_find(const char *name,
-					   unsigned int required_features)
+static struct elevator_type *__elevator_find(const char *name)
 {
 	struct elevator_type *e;
 
-	list_for_each_entry(e, &elv_list, list) {
-		if (elevator_match(e, name, required_features))
+	list_for_each_entry(e, &elv_list, list)
+		if (elevator_match(e, name))
 			return e;
-	}
-
 	return NULL;
 }
 
@@ -138,8 +121,8 @@ static struct elevator_type *elevator_find_get(struct request_queue *q,
 	struct elevator_type *e;
 
 	spin_lock(&elv_list_lock);
-	e = elevator_find(name, q->required_elevator_features);
-	if (e && !elevator_tryget(e))
+	e = __elevator_find(name);
+	if (e && (!elv_support_features(q, e) || !elevator_tryget(e)))
 		e = NULL;
 	spin_unlock(&elv_list_lock);
 	return e;
@@ -633,8 +616,7 @@ static struct elevator_type *elevator_get_by_features(struct request_queue *q)
 	spin_lock(&elv_list_lock);
 
 	list_for_each_entry(e, &elv_list, list) {
-		if (elv_support_features(e->elevator_features,
-					 q->required_elevator_features)) {
+		if (elv_support_features(q, e)) {
 			found = e;
 			break;
 		}
@@ -739,7 +721,7 @@ static int elevator_change(struct request_queue *q, const char *elevator_name)
 		return elevator_switch(q, NULL);
 	}
 
-	if (q->elevator && elevator_match(q->elevator->type, elevator_name, 0))
+	if (q->elevator && elevator_match(q->elevator->type, elevator_name))
 		return 0;
 
 	e = elevator_find_get(q, elevator_name);
@@ -790,8 +772,7 @@ ssize_t elv_iosched_show(struct request_queue *q, char *name)
 			len += sprintf(name+len, "[%s] ", cur->elevator_name);
 			continue;
 		}
-		if (elevator_match(e, e->elevator_name,
-				   q->required_elevator_features))
+		if (elv_support_features(q, e))
 			len += sprintf(name+len, "%s ", e->elevator_name);
 	}
 	spin_unlock(&elv_list_lock);
-- 
2.30.2

[PATCH 7/7] block: split elevator_switch

[Christoph Hellwig]

Split an elevator_disable helper from elevator_switch for the case where
we want to switch to no scheduler at all.  This includes removing the
pointless elevator_switch_mq helper and removing the switch to no
schedule logic from blk_mq_init_sched.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-mq-sched.c |  7 ----
 block/blk-mq.c       |  2 +-
 block/blk.h          |  1 +
 block/elevator.c     | 77 ++++++++++++++++++++++----------------------
 4 files changed, 40 insertions(+), 47 deletions(-)

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 68227240fdea3..23d1a90fec427 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -564,13 +564,6 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
 	unsigned long i;
 	int ret;
 
-	if (!e) {
-		blk_queue_flag_clear(QUEUE_FLAG_SQ_SCHED, q);
-		q->elevator = NULL;
-		q->nr_requests = q->tag_set->queue_depth;
-		return 0;
-	}
-
 	/*
 	 * Default to double of smaller one between hw queue_depth and 128,
 	 * since we don't split into sync/async like the old code did.
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4cecf281123f6..1ebb2e68ac66f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4556,7 +4556,7 @@ static bool blk_mq_elv_switch_none(struct list_head *head,
 	__elevator_get(qe->type);
 	qe->type = q->elevator->type;
 	list_add(&qe->node, head);
-	elevator_switch(q, NULL);
+	elevator_disable(q);
 	mutex_unlock(&q->sysfs_lock);
 
 	return true;
diff --git a/block/blk.h b/block/blk.h
index 7f9e089ab1f75..f1398fb96cec9 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -278,6 +278,7 @@ bool blk_bio_list_merge(struct request_queue *q, struct list_head *list,
 void blk_insert_flush(struct request *rq);
 
 int elevator_switch(struct request_queue *q, struct elevator_type *new_e);
+void elevator_disable(struct request_queue *q);
 void elevator_exit(struct request_queue *q);
 int elv_register_queue(struct request_queue *q, bool uevent);
 void elv_unregister_queue(struct request_queue *q);
diff --git a/block/elevator.c b/block/elevator.c
index 4042e524333e0..6fdbfca1bc61e 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -548,39 +548,6 @@ void elv_unregister(struct elevator_type *e)
 }
 EXPORT_SYMBOL_GPL(elv_unregister);
 
-static int elevator_switch_mq(struct request_queue *q,
-			      struct elevator_type *new_e)
-{
-	int ret;
-
-	lockdep_assert_held(&q->sysfs_lock);
-
-	if (q->elevator) {
-		elv_unregister_queue(q);
-		elevator_exit(q);
-	}
-
-	ret = blk_mq_init_sched(q, new_e);
-	if (ret)
-		goto out;
-
-	if (new_e) {
-		ret = elv_register_queue(q, true);
-		if (ret) {
-			elevator_exit(q);
-			goto out;
-		}
-	}
-
-	if (new_e)
-		blk_add_trace_msg(q, "elv switch: %s", new_e->elevator_name);
-	else
-		blk_add_trace_msg(q, "elv switch: none");
-
-out:
-	return ret;
-}
-
 static inline bool elv_support_iosched(struct request_queue *q)
 {
 	if (!queue_is_mq(q) ||
@@ -685,19 +652,51 @@ void elevator_init_mq(struct request_queue *q)
  */
 int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
 {
-	int err;
+	int ret;
 
 	lockdep_assert_held(&q->sysfs_lock);
 
 	blk_mq_freeze_queue(q);
 	blk_mq_quiesce_queue(q);
 
-	err = elevator_switch_mq(q, new_e);
+	if (q->elevator) {
+		elv_unregister_queue(q);
+		elevator_exit(q);
+	}
 
+	ret = blk_mq_init_sched(q, new_e);
+	if (ret)
+		goto out_unfreeze;
+
+	ret = elv_register_queue(q, true);
+	if (ret) {
+		elevator_exit(q);
+		goto out_unfreeze;
+	}
+	blk_add_trace_msg(q, "elv switch: %s", new_e->elevator_name);
+
+out_unfreeze:
 	blk_mq_unquiesce_queue(q);
 	blk_mq_unfreeze_queue(q);
+	return ret;
+}
+
+void elevator_disable(struct request_queue *q)
+{
+	lockdep_assert_held(&q->sysfs_lock);
 
-	return err;
+	blk_mq_freeze_queue(q);
+	blk_mq_quiesce_queue(q);
+
+	elv_unregister_queue(q);
+	elevator_exit(q);
+	blk_queue_flag_clear(QUEUE_FLAG_SQ_SCHED, q);
+	q->elevator = NULL;
+	q->nr_requests = q->tag_set->queue_depth;
+	blk_add_trace_msg(q, "elv switch: none");
+
+	blk_mq_unquiesce_queue(q);
+	blk_mq_unfreeze_queue(q);
 }
 
 /*
@@ -716,9 +715,9 @@ static int elevator_change(struct request_queue *q, const char *elevator_name)
 	 * Special case for mq, turn off scheduling
 	 */
 	if (!strncmp(elevator_name, "none", 4)) {
-		if (!q->elevator)
-			return 0;
-		return elevator_switch(q, NULL);
+		if (q->elevator)
+			elevator_disable(q);
+		return 0;
 	}
 
 	if (q->elevator && elevator_match(q->elevator->type, elevator_name))
-- 
2.30.2

Re: misc elevator code cleanups

[Jinlong Chen]

Hi, Christoph!

Only elevator_find_get is calling __elevator_find after applying the
series. Maybe we can just remove __elevator_find and move the list
iterating logic into elevator_find_get?

Thanks!
Jinlong Chen

Re: misc elevator code cleanups

[Christoph Hellwig]

On Sun, Oct 30, 2022 at 10:03:57PM +0800, Jinlong Chen wrote:
> Hi, Christoph!
> 
> Only elevator_find_get is calling __elevator_find after applying the
> series. Maybe we can just remove __elevator_find and move the list
> iterating logic into elevator_find_get?

We could. But then we'd need another local variable to track what
was found, so I'm not sure it is a win.  In general having a pure
list lookup in a helper while all the locking and refcounting in
a wrapper around it tends to be a quite nice pattern.

Re: Re: misc elevator code cleanups

[Jinlong Chen]

> > Only elevator_find_get is calling __elevator_find after applying the
> > series. Maybe we can just remove __elevator_find and move the list
> > iterating logic into elevator_find_get?
> 
> We could. But then we'd need another local variable to track what
> was found, so I'm not sure it is a win.  In general having a pure
> list lookup in a helper while all the locking and refcounting in
> a wrapper around it tends to be a quite nice pattern.

Got it, thank you for your answer and patience!

Thanks!
Jinlong Chen

Re: (subset) misc elevator code cleanups

[Jens Axboe]

On Sun, 30 Oct 2022 11:07:07 +0100, Christoph Hellwig wrote:
> this series has a bunch of random elevator cleanups.
> 
> Diffstat:
>  blk-mq-sched.c |    7 --
>  blk-mq.c       |    2
>  blk.h          |    1
>  elevator.c     |  175 +++++++++++++++++++++++----------------------------------
>  4 files changed, 73 insertions(+), 112 deletions(-)
> 
> [...]

Applied, thanks!

[2/7] block: cleanup elevator_get
      commit: 81eaca442ea962c43bdb1e9cbb9eddb41b97491d
[3/7] block: exit elv_iosched_show early when I/O schedulers are not supported
      commit: aae2a643f508d768b65e59da447f3b11688db3cf
[4/7] block: cleanup the variable naming in elv_iosched_store
      commit: 16095af2fa2c3089ff1162e677d6596772f6f478
[5/7] block: simplify the check for the current elevator in elv_iosched_show
      commit: 2eef17a209ab4d77923222045d462d379d6ef692
[6/7] block: don't check for required features in elevator_match
      commit: bc3caee659d70addf58dde216d10a5589ab9ec73
[7/7] block: split elevator_switch
      commit: 817b4eed4a21cda7dad3002b23901156abdb7c36

Best regards,
-- 
Jens Axboe