elevator refcount fixes

elevator refcount fixes

[Christoph Hellwig]

Hi Jens,

this series is a take on the elevator refcount fixes from Jinlong.
I've added a cleanup patch, and one that improves on one of the incidental
fixes he did, as well as splitting the main patch into two and improving
some comments.

Diffstat:
 blk-mq-sched.c |    1 +
 blk-mq.c       |   13 ++++---------
 elevator.c     |   43 +++++++++++++++++++------------------------
 elevator.h     |   15 +++++++++++++++
 4 files changed, 39 insertions(+), 33 deletions(-)

[PATCH 1/4] block: add proper helpers for elevator_type module refcount management

[Christoph Hellwig]

Make sure we have helpers for all relevant module refcount operations on
the elevator_type in elevator.h, and use them.  Move the call to the get
helper in blk_mq_elv_switch_none a bit so that it is obvious with a less
verbose comment.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-mq.c   | 11 ++---------
 block/elevator.c |  9 ++-------
 block/elevator.h | 15 +++++++++++++++
 3 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 33292c01875d5..9db8814cdd02b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4554,17 +4554,10 @@ static bool blk_mq_elv_switch_none(struct list_head *head,
 
 	INIT_LIST_HEAD(&qe->node);
 	qe->q = q;
+	/* keep a reference to the elevator module as we'll switch back */
+	__elevator_get(qe->type);
 	qe->type = q->elevator->type;
 	list_add(&qe->node, head);
-
-	/*
-	 * After elevator_switch, the previous elevator_queue will be
-	 * released by elevator_release. The reference of the io scheduler
-	 * module get by elevator_get will also be put. So we need to get
-	 * a reference of the io scheduler module here to prevent it to be
-	 * removed.
-	 */
-	__module_get(qe->type->elevator_owner);
 	elevator_switch(q, NULL);
 	mutex_unlock(&q->sysfs_lock);
 
diff --git a/block/elevator.c b/block/elevator.c
index bd71f0fc4e4b6..40ba43aa9ece0 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -132,11 +132,6 @@ static struct elevator_type *elevator_find(const char *name,
 	return NULL;
 }
 
-static void elevator_put(struct elevator_type *e)
-{
-	module_put(e->elevator_owner);
-}
-
 static struct elevator_type *elevator_get(struct request_queue *q,
 					  const char *name, bool try_loading)
 {
@@ -152,7 +147,7 @@ static struct elevator_type *elevator_get(struct request_queue *q,
 		e = elevator_find(name, q->required_elevator_features);
 	}
 
-	if (e && !try_module_get(e->elevator_owner))
+	if (e && !elevator_tryget(e))
 		e = NULL;
 
 	spin_unlock(&elv_list_lock);
@@ -663,7 +658,7 @@ static struct elevator_type *elevator_get_by_features(struct request_queue *q)
 		}
 	}
 
-	if (found && !try_module_get(found->elevator_owner))
+	if (found && !elevator_tryget(found))
 		found = NULL;
 
 	spin_unlock(&elv_list_lock);
diff --git a/block/elevator.h b/block/elevator.h
index 3f0593b3bf9d3..44e5f35fa740d 100644
--- a/block/elevator.h
+++ b/block/elevator.h
@@ -84,6 +84,21 @@ struct elevator_type
 	struct list_head list;
 };
 
+static inline bool elevator_tryget(struct elevator_type *e)
+{
+	return try_module_get(e->elevator_owner);
+}
+
+static inline void __elevator_get(struct elevator_type *e)
+{
+	__module_get(e->elevator_owner);
+}
+
+static inline void elevator_put(struct elevator_type *e)
+{
+	module_put(e->elevator_owner);
+}
+
 #define ELV_HASH_BITS 6
 
 void elv_rqhash_del(struct request_queue *q, struct request *rq);
-- 
2.30.2

[PATCH 2/4] block: sanitize the elevator name before passing it to __elevator_change

[Christoph Hellwig]

The stripped name should also be used for the none check.  To do so
strip it in the caller and pass in the sanitized name.  Drop the pointless
__ prefix in the function name while we're at it.

Based on a patch from Jinlong Chen <nickyc975@zju.edu.cn>.

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

diff --git a/block/elevator.c b/block/elevator.c
index 40ba43aa9ece0..b7f098f735b6b 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -738,9 +738,8 @@ int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
 /*
  * Switch this queue to the given IO scheduler.
  */
-static int __elevator_change(struct request_queue *q, const char *name)
+static int elevator_change(struct request_queue *q, const char *elevator_name)
 {
-	char elevator_name[ELV_NAME_MAX];
 	struct elevator_type *e;
 
 	/* Make sure queue is not in the middle of being removed */
@@ -750,14 +749,13 @@ static int __elevator_change(struct request_queue *q, const char *name)
 	/*
 	 * Special case for mq, turn off scheduling
 	 */
-	if (!strncmp(name, "none", 4)) {
+	if (!strncmp(elevator_name, "none", 4)) {
 		if (!q->elevator)
 			return 0;
 		return elevator_switch(q, NULL);
 	}
 
-	strlcpy(elevator_name, name, sizeof(elevator_name));
-	e = elevator_get(q, strstrip(elevator_name), true);
+	e = elevator_get(q, elevator_name, true);
 	if (!e)
 		return -EINVAL;
 
@@ -770,18 +768,19 @@ static int __elevator_change(struct request_queue *q, const char *name)
 	return elevator_switch(q, e);
 }
 
-ssize_t elv_iosched_store(struct request_queue *q, const char *name,
+ssize_t elv_iosched_store(struct request_queue *q, const char *buf,
 			  size_t count)
 {
+	char elevator_name[ELV_NAME_MAX];
 	int ret;
 
 	if (!elv_support_iosched(q))
 		return count;
 
-	ret = __elevator_change(q, name);
+	strlcpy(elevator_name, buf, sizeof(elevator_name));
+	ret = elevator_change(q, strstrip(elevator_name));
 	if (!ret)
 		return count;
-
 	return ret;
 }
 
-- 
2.30.2

[PATCH 3/4] block: check for an unchanged elevator earlier in __elevator_change

[Christoph Hellwig]

From: Jinlong Chen <nickyc975@zju.edu.cn>

No need to find the actual elevator_type struct for this comparism,
the name is all that is needed.

Signed-off-by: Jinlong Chen <nickyc975@zju.edu.cn>
[hch: split from a larger patch]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/elevator.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index b7f098f735b6b..887becfe6a8d4 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -755,16 +755,13 @@ 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))
+		return 0;
+
 	e = elevator_get(q, elevator_name, true);
 	if (!e)
 		return -EINVAL;
 
-	if (q->elevator &&
-	    elevator_match(q->elevator->type, elevator_name, 0)) {
-		elevator_put(e);
-		return 0;
-	}
-
 	return elevator_switch(q, e);
 }
 
-- 
2.30.2

[PATCH 4/4] block: fix up elevator_type refcounting

[Christoph Hellwig]

From: Jinlong Chen <nickyc975@zju.edu.cn>

The current reference management logic of io scheduler modules contains
refcnt problems. For example, blk_mq_init_sched may fail before or after
the calling of e->ops.init_sched. If it fails before the calling, it does
nothing to the reference to the io scheduler module. But if it fails after
the calling, it releases the reference by calling kobject_put(&eq->kobj).

As the callers of blk_mq_init_sched can't know exactly where the failure
happens, they can't handle the reference to the io scheduler module
properly: releasing the reference on failure results in double-release if
blk_mq_init_sched has released it, and not releasing the reference results
in ghost reference if blk_mq_init_sched did not release it either.

The same problem also exists in io schedulers' init_sched implementations.

We can address the problem by adding releasing statements to the error
handling procedures of blk_mq_init_sched and init_sched implementations.
But that is counterintuitive and requires modifications to existing io
schedulers.

Instead, We make elevator_alloc get the io scheduler module references
that will be released by elevator_release. And then, we match each
elevator_get with an elevator_put. Therefore, each reference to an io
scheduler module explicitly has its own getter and releaser, and we no
longer need to worry about the refcnt problems.

The bugs and the patch can be validated with tools here:
https://github.com/nickyc975/linux_elv_refcnt_bug.git

Signed-off-by: Jinlong Chen <nickyc975@zju.edu.cn>
[hch: split out a few bits into separate patches, use a non-try
      module_get in elevator_alloc]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-mq-sched.c |  1 +
 block/blk-mq.c       |  2 ++
 block/elevator.c     | 10 +++++++---
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index a4f7c101b53b2..68227240fdea3 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -555,6 +555,7 @@ static int blk_mq_init_sched_shared_tags(struct request_queue *queue)
 	return 0;
 }
 
+/* caller must have a reference to @e, will grab another one if successful */
 int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
 {
 	unsigned int flags = q->tag_set->flags;
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 9db8814cdd02b..098432d3caf1c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4591,6 +4591,8 @@ static void blk_mq_elv_switch_back(struct list_head *head,
 
 	mutex_lock(&q->sysfs_lock);
 	elevator_switch(q, t);
+	/* drop the reference acquired in blk_mq_elv_switch_none */
+	elevator_put(t);
 	mutex_unlock(&q->sysfs_lock);
 }
 
diff --git a/block/elevator.c b/block/elevator.c
index 887becfe6a8d4..e36d132b47e1e 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -165,6 +165,7 @@ struct elevator_queue *elevator_alloc(struct request_queue *q,
 	if (unlikely(!eq))
 		return NULL;
 
+	__elevator_get(e);
 	eq->type = e;
 	kobject_init(&eq->kobj, &elv_ktype);
 	mutex_init(&eq->sysfs_lock);
@@ -708,8 +709,9 @@ void elevator_init_mq(struct request_queue *q)
 	if (err) {
 		pr_warn("\"%s\" elevator initialization failed, "
 			"falling back to \"none\"\n", e->elevator_name);
-		elevator_put(e);
 	}
+
+	elevator_put(e);
 }
 
 /*
@@ -741,6 +743,7 @@ int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
 static int elevator_change(struct request_queue *q, const char *elevator_name)
 {
 	struct elevator_type *e;
+	int ret;
 
 	/* Make sure queue is not in the middle of being removed */
 	if (!blk_queue_registered(q))
@@ -761,8 +764,9 @@ static int elevator_change(struct request_queue *q, const char *elevator_name)
 	e = elevator_get(q, elevator_name, true);
 	if (!e)
 		return -EINVAL;
-
-	return elevator_switch(q, e);
+	ret = elevator_switch(q, e);
+	elevator_put(e);
+	return ret;
 }
 
 ssize_t elv_iosched_store(struct request_queue *q, const char *buf,
-- 
2.30.2

Re: elevator refcount fixes

[Jinlong Chen]

> 
> this series is a take on the elevator refcount fixes from Jinlong.
> I've added a cleanup patch, and one that improves on one of the incidental
> fixes he did, as well as splitting the main patch into two and improving
> some comments.
> 

The series is much clearer than my original long patch. Thank you Christoph!

Jinlong Chen

Re: elevator refcount fixes

[Jens Axboe]

On Thu, 20 Oct 2022 08:48:15 +0200, Christoph Hellwig wrote:
> this series is a take on the elevator refcount fixes from Jinlong.
> I've added a cleanup patch, and one that improves on one of the incidental
> fixes he did, as well as splitting the main patch into two and improving
> some comments.
> 
> Diffstat:
>  blk-mq-sched.c |    1 +
>  blk-mq.c       |   13 ++++---------
>  elevator.c     |   43 +++++++++++++++++++------------------------
>  elevator.h     |   15 +++++++++++++++
>  4 files changed, 39 insertions(+), 33 deletions(-)
> 
> [...]

Applied, thanks!

[1/4] block: add proper helpers for elevator_type module refcount management
      commit: 61e1f359c2bc78360cf9741959918934d9362aa5
[2/4] block: sanitize the elevator name before passing it to __elevator_change
      commit: d1368d8c074010b221be1e5474e0d318567bbec7
[3/4] block: check for an unchanged elevator earlier in __elevator_change
      commit: ffd37387225f6ccd47765dc33fc56db14a7a8487
[4/4] block: fix up elevator_type refcounting
      commit: bcd3010074ec9dc1bd65c210a2ec3815dc653340

Best regards,
-- 
Jens Axboe