* misc elevator code cleanups
@ 2022-10-30 10:07 Christoph Hellwig
2022-10-30 10:07 ` [PATCH 1/7] block: drop the duplicate check in elv_register Christoph Hellwig
` (8 more replies)
0 siblings, 9 replies; 14+ messages in thread
From: Christoph Hellwig @ 2022-10-30 10:07 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block
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(-)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/7] block: drop the duplicate check in elv_register
2022-10-30 10:07 misc elevator code cleanups Christoph Hellwig
@ 2022-10-30 10:07 ` Christoph Hellwig
2022-10-31 13:50 ` Jens Axboe
2022-10-30 10:07 ` [PATCH 2/7] block: cleanup elevator_get Christoph Hellwig
` (7 subsequent siblings)
8 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2022-10-30 10:07 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/7] block: cleanup elevator_get
2022-10-30 10:07 misc elevator code cleanups Christoph Hellwig
2022-10-30 10:07 ` [PATCH 1/7] block: drop the duplicate check in elv_register Christoph Hellwig
@ 2022-10-30 10:07 ` Christoph Hellwig
2022-10-30 10:07 ` [PATCH 3/7] block: exit elv_iosched_show early when I/O schedulers are not supported Christoph Hellwig
` (6 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2022-10-30 10:07 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/7] block: exit elv_iosched_show early when I/O schedulers are not supported
2022-10-30 10:07 misc elevator code cleanups Christoph Hellwig
2022-10-30 10:07 ` [PATCH 1/7] block: drop the duplicate check in elv_register Christoph Hellwig
2022-10-30 10:07 ` [PATCH 2/7] block: cleanup elevator_get Christoph Hellwig
@ 2022-10-30 10:07 ` Christoph Hellwig
2022-10-30 10:07 ` [PATCH 4/7] block: cleanup the variable naming in elv_iosched_store Christoph Hellwig
` (5 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2022-10-30 10:07 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/7] block: cleanup the variable naming in elv_iosched_store
2022-10-30 10:07 misc elevator code cleanups Christoph Hellwig
` (2 preceding siblings ...)
2022-10-30 10:07 ` [PATCH 3/7] block: exit elv_iosched_show early when I/O schedulers are not supported Christoph Hellwig
@ 2022-10-30 10:07 ` Christoph Hellwig
2022-10-30 10:07 ` [PATCH 5/7] block: simplify the check for the current elevator in elv_iosched_show Christoph Hellwig
` (4 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2022-10-30 10:07 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/7] block: simplify the check for the current elevator in elv_iosched_show
2022-10-30 10:07 misc elevator code cleanups Christoph Hellwig
` (3 preceding siblings ...)
2022-10-30 10:07 ` [PATCH 4/7] block: cleanup the variable naming in elv_iosched_store Christoph Hellwig
@ 2022-10-30 10:07 ` Christoph Hellwig
2022-10-30 10:07 ` [PATCH 6/7] block: don't check for required features in elevator_match Christoph Hellwig
` (3 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2022-10-30 10:07 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 6/7] block: don't check for required features in elevator_match
2022-10-30 10:07 misc elevator code cleanups Christoph Hellwig
` (4 preceding siblings ...)
2022-10-30 10:07 ` [PATCH 5/7] block: simplify the check for the current elevator in elv_iosched_show Christoph Hellwig
@ 2022-10-30 10:07 ` Christoph Hellwig
2022-10-30 10:07 ` [PATCH 7/7] block: split elevator_switch Christoph Hellwig
` (2 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2022-10-30 10:07 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 7/7] block: split elevator_switch
2022-10-30 10:07 misc elevator code cleanups Christoph Hellwig
` (5 preceding siblings ...)
2022-10-30 10:07 ` [PATCH 6/7] block: don't check for required features in elevator_match Christoph Hellwig
@ 2022-10-30 10:07 ` Christoph Hellwig
2022-10-30 14:03 ` misc elevator code cleanups Jinlong Chen
2022-11-01 14:03 ` (subset) " Jens Axboe
8 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2022-10-30 10:07 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: misc elevator code cleanups
2022-10-30 10:07 misc elevator code cleanups Christoph Hellwig
` (6 preceding siblings ...)
2022-10-30 10:07 ` [PATCH 7/7] block: split elevator_switch Christoph Hellwig
@ 2022-10-30 14:03 ` Jinlong Chen
2022-10-30 15:32 ` Christoph Hellwig
2022-11-01 14:03 ` (subset) " Jens Axboe
8 siblings, 1 reply; 14+ messages in thread
From: Jinlong Chen @ 2022-10-30 14:03 UTC (permalink / raw)
To: hch; +Cc: axboe, linux-block, 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: misc elevator code cleanups
2022-10-30 14:03 ` misc elevator code cleanups Jinlong Chen
@ 2022-10-30 15:32 ` Christoph Hellwig
2022-10-31 1:49 ` Jinlong Chen
0 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2022-10-30 15:32 UTC (permalink / raw)
To: Jinlong Chen; +Cc: hch, axboe, linux-block
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Re: misc elevator code cleanups
2022-10-30 15:32 ` Christoph Hellwig
@ 2022-10-31 1:49 ` Jinlong Chen
0 siblings, 0 replies; 14+ messages in thread
From: Jinlong Chen @ 2022-10-31 1:49 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: axboe, linux-block
> > 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/7] block: drop the duplicate check in elv_register
2022-10-30 10:07 ` [PATCH 1/7] block: drop the duplicate check in elv_register Christoph Hellwig
@ 2022-10-31 13:50 ` Jens Axboe
2022-11-01 10:02 ` Christoph Hellwig
0 siblings, 1 reply; 14+ messages in thread
From: Jens Axboe @ 2022-10-31 13:50 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/7] block: drop the duplicate check in elv_register
2022-10-31 13:50 ` Jens Axboe
@ 2022-11-01 10:02 ` Christoph Hellwig
0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2022-11-01 10:02 UTC (permalink / raw)
To: Jens Axboe; +Cc: Christoph Hellwig, linux-block
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: (subset) misc elevator code cleanups
2022-10-30 10:07 misc elevator code cleanups Christoph Hellwig
` (7 preceding siblings ...)
2022-10-30 14:03 ` misc elevator code cleanups Jinlong Chen
@ 2022-11-01 14:03 ` Jens Axboe
8 siblings, 0 replies; 14+ messages in thread
From: Jens Axboe @ 2022-11-01 14:03 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block
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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-11-01 14:03 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-30 10:07 misc elevator code cleanups Christoph Hellwig
2022-10-30 10:07 ` [PATCH 1/7] block: drop the duplicate check in elv_register Christoph Hellwig
2022-10-31 13:50 ` Jens Axboe
2022-11-01 10:02 ` Christoph Hellwig
2022-10-30 10:07 ` [PATCH 2/7] block: cleanup elevator_get Christoph Hellwig
2022-10-30 10:07 ` [PATCH 3/7] block: exit elv_iosched_show early when I/O schedulers are not supported Christoph Hellwig
2022-10-30 10:07 ` [PATCH 4/7] block: cleanup the variable naming in elv_iosched_store Christoph Hellwig
2022-10-30 10:07 ` [PATCH 5/7] block: simplify the check for the current elevator in elv_iosched_show Christoph Hellwig
2022-10-30 10:07 ` [PATCH 6/7] block: don't check for required features in elevator_match Christoph Hellwig
2022-10-30 10:07 ` [PATCH 7/7] block: split elevator_switch Christoph Hellwig
2022-10-30 14:03 ` misc elevator code cleanups Jinlong Chen
2022-10-30 15:32 ` Christoph Hellwig
2022-10-31 1:49 ` Jinlong Chen
2022-11-01 14:03 ` (subset) " Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).