[PATCH 0/3] cleanups for queue freezing functions

[PATCH 0/3] cleanups for queue freezing functions

[Jinlong Chen]

This series of patches cleans up non-blk-mq functions in blk-mq.c to build
a consistent blk_mq_* namespace.

Jinlong Chen (3):
  blk-mq: remove redundant call to blk_freeze_queue_start in
    blk_mq_destroy_queue
  blk-mq: remove blk_freeze_queue
  block: hide back blk_freeze_queue_start and export its blk-mq alias

 block/blk-core.c              | 13 +++++++++
 block/blk-mq.c                | 52 ++++++++++++++---------------------
 block/blk-pm.c                |  2 +-
 block/blk.h                   |  2 +-
 drivers/nvme/host/core.c      |  2 +-
 drivers/nvme/host/multipath.c |  2 +-
 include/linux/blk-mq.h        |  2 +-
 7 files changed, 39 insertions(+), 36 deletions(-)

-- 
2.31.1

[PATCH 1/3] blk-mq: remove redundant call to blk_freeze_queue_start in blk_mq_destroy_queue

[Jinlong Chen]

Calling blk_freeze_queue results in a redundant call to
blk_freeze_queue_start as it has been called in blk_queue_start_drain.

Replace blk_freeze_queue with blk_mq_freeze_queue_wait to avoid the
redundant call.

Signed-off-by: Jinlong Chen <nickyc975@zju.edu.cn>
---
 block/blk-mq.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4cecf281123f..14c4165511b9 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4005,7 +4005,12 @@ void blk_mq_destroy_queue(struct request_queue *q)
 
 	blk_queue_flag_set(QUEUE_FLAG_DYING, q);
 	blk_queue_start_drain(q);
-	blk_freeze_queue(q);
+
+	/*
+	 * blk_freeze_queue_start has been called in blk_queue_start_drain, we just
+	 * need to wait.
+	 */
+	blk_mq_freeze_queue_wait(q);
 
 	blk_sync_queue(q);
 	blk_mq_cancel_work_sync(q);
-- 
2.31.1

[PATCH 2/3] blk-mq: remove blk_freeze_queue

[Jinlong Chen]

Nobody is calling blk_freeze_queue except its alias, so remove it.

Signed-off-by: Jinlong Chen <nickyc975@zju.edu.cn>
---
 block/blk-mq.c | 18 +-----------------
 block/blk.h    |  1 -
 2 files changed, 1 insertion(+), 18 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 14c4165511b9..e0654a2e80b9 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -194,27 +194,11 @@ EXPORT_SYMBOL_GPL(blk_mq_freeze_queue_wait_timeout);
  * Guarantee no request is in use, so we can change any data structure of
  * the queue afterward.
  */
-void blk_freeze_queue(struct request_queue *q)
+void blk_mq_freeze_queue(struct request_queue *q)
 {
-	/*
-	 * In the !blk_mq case we are only calling this to kill the
-	 * q_usage_counter, otherwise this increases the freeze depth
-	 * and waits for it to return to zero.  For this reason there is
-	 * no blk_unfreeze_queue(), and blk_freeze_queue() is not
-	 * exported to drivers as the only user for unfreeze is blk_mq.
-	 */
 	blk_freeze_queue_start(q);
 	blk_mq_freeze_queue_wait(q);
 }
-
-void blk_mq_freeze_queue(struct request_queue *q)
-{
-	/*
-	 * ...just an alias to keep freeze and unfreeze actions balanced
-	 * in the blk_mq_* namespace
-	 */
-	blk_freeze_queue(q);
-}
 EXPORT_SYMBOL_GPL(blk_mq_freeze_queue);
 
 void __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic)
diff --git a/block/blk.h b/block/blk.h
index 7f9e089ab1f7..e9addea2838a 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -37,7 +37,6 @@ struct blk_flush_queue *blk_alloc_flush_queue(int node, int cmd_size,
 					      gfp_t flags);
 void blk_free_flush_queue(struct blk_flush_queue *q);
 
-void blk_freeze_queue(struct request_queue *q);
 void __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic);
 void blk_queue_start_drain(struct request_queue *q);
 int __bio_queue_enter(struct request_queue *q, struct bio *bio);
-- 
2.31.1

[PATCH 3/3] block: hide back blk_freeze_queue_start and export its blk-mq alias

[Jinlong Chen]

blk_freeze_queue_start is used internally for universal queue draining and
externally for blk-mq specific queue freezing. Keep the non-blk-mq name
private and export a blk-mq alias to users.

Signed-off-by: Jinlong Chen <nickyc975@zju.edu.cn>
---
 block/blk-core.c              | 13 +++++++++++++
 block/blk-mq.c                | 27 ++++++++++++++-------------
 block/blk-pm.c                |  2 +-
 block/blk.h                   |  1 +
 drivers/nvme/host/core.c      |  2 +-
 drivers/nvme/host/multipath.c |  2 +-
 include/linux/blk-mq.h        |  2 +-
 7 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 5d50dd16e2a5..d3dd439a8ed4 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -269,6 +269,19 @@ void blk_put_queue(struct request_queue *q)
 }
 EXPORT_SYMBOL(blk_put_queue);
 
+void blk_freeze_queue_start(struct request_queue *q)
+{
+	mutex_lock(&q->mq_freeze_lock);
+	if (++q->mq_freeze_depth == 1) {
+		percpu_ref_kill(&q->q_usage_counter);
+		mutex_unlock(&q->mq_freeze_lock);
+		if (queue_is_mq(q))
+			blk_mq_run_hw_queues(q, false);
+	} else {
+		mutex_unlock(&q->mq_freeze_lock);
+	}
+}
+
 void blk_queue_start_drain(struct request_queue *q)
 {
 	/*
diff --git a/block/blk-mq.c b/block/blk-mq.c
index e0654a2e80b9..d638bd0fb4d8 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -161,19 +161,20 @@ void blk_mq_in_flight_rw(struct request_queue *q, struct block_device *part,
 	inflight[1] = mi.inflight[1];
 }
 
-void blk_freeze_queue_start(struct request_queue *q)
+void blk_mq_freeze_queue_start(struct request_queue *q)
 {
-	mutex_lock(&q->mq_freeze_lock);
-	if (++q->mq_freeze_depth == 1) {
-		percpu_ref_kill(&q->q_usage_counter);
-		mutex_unlock(&q->mq_freeze_lock);
-		if (queue_is_mq(q))
-			blk_mq_run_hw_queues(q, false);
-	} else {
-		mutex_unlock(&q->mq_freeze_lock);
-	}
+	/*
+	 * Warn on non-blk-mq usages.
+	 */
+	WARN_ON_ONCE(!queue_is_mq(q));
+
+	/*
+	 * Just an alias of blk_freeze_queue_start to keep the consistency of the
+	 * blk_mq_* namespace.
+	 */
+	blk_freeze_queue_start(q);
 }
-EXPORT_SYMBOL_GPL(blk_freeze_queue_start);
+EXPORT_SYMBOL_GPL(blk_mq_freeze_queue_start);
 
 void blk_mq_freeze_queue_wait(struct request_queue *q)
 {
@@ -196,7 +197,7 @@ EXPORT_SYMBOL_GPL(blk_mq_freeze_queue_wait_timeout);
  */
 void blk_mq_freeze_queue(struct request_queue *q)
 {
-	blk_freeze_queue_start(q);
+	blk_mq_freeze_queue_start(q);
 	blk_mq_freeze_queue_wait(q);
 }
 EXPORT_SYMBOL_GPL(blk_mq_freeze_queue);
@@ -1570,7 +1571,7 @@ static void blk_mq_timeout_work(struct work_struct *work)
 	 * percpu_ref_tryget directly, because we need to be able to
 	 * obtain a reference even in the short window between the queue
 	 * starting to freeze, by dropping the first reference in
-	 * blk_freeze_queue_start, and the moment the last request is
+	 * blk_mq_freeze_queue_start, and the moment the last request is
 	 * consumed, marked by the instant q_usage_counter reaches
 	 * zero.
 	 */
diff --git a/block/blk-pm.c b/block/blk-pm.c
index 2dad62cc1572..ae2b950ed45d 100644
--- a/block/blk-pm.c
+++ b/block/blk-pm.c
@@ -80,7 +80,7 @@ int blk_pre_runtime_suspend(struct request_queue *q)
 	blk_set_pm_only(q);
 	ret = -EBUSY;
 	/* Switch q_usage_counter from per-cpu to atomic mode. */
-	blk_freeze_queue_start(q);
+	blk_mq_freeze_queue_start(q);
 	/*
 	 * Wait until atomic mode has been reached. Since that
 	 * involves calling call_rcu(), it is guaranteed that later
diff --git a/block/blk.h b/block/blk.h
index e9addea2838a..ee576bb74382 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -37,6 +37,7 @@ struct blk_flush_queue *blk_alloc_flush_queue(int node, int cmd_size,
 					      gfp_t flags);
 void blk_free_flush_queue(struct blk_flush_queue *q);
 
+void blk_freeze_queue_start(struct request_queue *q);
 void __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic);
 void blk_queue_start_drain(struct request_queue *q);
 int __bio_queue_enter(struct request_queue *q, struct bio *bio);
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0090dc0b3ae6..e2d5c54c651a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -5199,7 +5199,7 @@ void nvme_start_freeze(struct nvme_ctrl *ctrl)
 
 	down_read(&ctrl->namespaces_rwsem);
 	list_for_each_entry(ns, &ctrl->namespaces, list)
-		blk_freeze_queue_start(ns->queue);
+		blk_mq_freeze_queue_start(ns->queue);
 	up_read(&ctrl->namespaces_rwsem);
 }
 EXPORT_SYMBOL_GPL(nvme_start_freeze);
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 0ea7e441e080..3bb358bd0cde 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -77,7 +77,7 @@ void nvme_mpath_start_freeze(struct nvme_subsystem *subsys)
 	lockdep_assert_held(&subsys->lock);
 	list_for_each_entry(h, &subsys->nsheads, entry)
 		if (h->disk)
-			blk_freeze_queue_start(h->disk->queue);
+			blk_mq_freeze_queue_start(h->disk->queue);
 }
 
 void nvme_failover_req(struct request *req)
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 569053ed959d..8600d4b4aa80 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -887,7 +887,7 @@ void blk_mq_tagset_busy_iter(struct blk_mq_tag_set *tagset,
 void blk_mq_tagset_wait_completed_request(struct blk_mq_tag_set *tagset);
 void blk_mq_freeze_queue(struct request_queue *q);
 void blk_mq_unfreeze_queue(struct request_queue *q);
-void blk_freeze_queue_start(struct request_queue *q);
+void blk_mq_freeze_queue_start(struct request_queue *q);
 void blk_mq_freeze_queue_wait(struct request_queue *q);
 int blk_mq_freeze_queue_wait_timeout(struct request_queue *q,
 				     unsigned long timeout);
-- 
2.31.1

Re: [PATCH 1/3] blk-mq: remove redundant call to blk_freeze_queue_start in blk_mq_destroy_queue

[Bart Van Assche]

On 10/29/22 03:02, Jinlong Chen wrote:
> Calling blk_freeze_queue results in a redundant call to
> blk_freeze_queue_start as it has been called in blk_queue_start_drain.
> 
> Replace blk_freeze_queue with blk_mq_freeze_queue_wait to avoid the
> redundant call.

blk_mq_destroy_queue() has more callers than blk_queue_start_drain() so 
the above description is at least misleading.

Additionally, the word "cleanup" from the patch series title indicates 
that no patch in this series changes the behavior of the code. This 
patch involves a behavior change.

I think this patch introduces a hang for every caller of 
blk_mq_destroy_queue() other than blk_queue_start_drain().

Bart.

Re: Re: [PATCH 1/3] blk-mq: remove redundant call to blk_freeze_queue_start in blk_mq_destroy_queue

[Jinlong Chen]

Hi, Bart.

> > Calling blk_freeze_queue results in a redundant call to
> > blk_freeze_queue_start as it has been called in blk_queue_start_drain.
> > 
> > Replace blk_freeze_queue with blk_mq_freeze_queue_wait to avoid the
> > redundant call.
> 
> blk_mq_destroy_queue() has more callers than blk_queue_start_drain() so 
> the above description is at least misleading.
> 
> Additionally, the word "cleanup" from the patch series title indicates 
> that no patch in this series changes the behavior of the code. This 
> patch involves a behavior change.

Sorry for my poor description. I'll send a new series with these
description problems resolved.

> I think this patch introduces a hang for every caller of 
> blk_mq_destroy_queue() other than blk_queue_start_drain().
> 
> Bart.

I don't see why the patch introduces a hang. The calling relationship in
blk_mq_destroy_queue is as follows:

blk_mq_destroy_queue()
    ...
    -> blk_queue_start_drain()
        -> blk_freeze_queue_start()  <- called
        ...
    -> blk_freeze_queue()
        -> blk_freeze_queue_start()  <- called again
        -> blk_mq_freeze_queue_wait()
    ...

So I think there is a redundant call to blk_freeze_queue_start(), we
just need to call blk_mq_freeze_queue_wait() after calling
blk_queue_start_drain().

Thanks!
Jinlong Chen

Re: [PATCH 1/3] blk-mq: remove redundant call to blk_freeze_queue_start in blk_mq_destroy_queue

[Bart Van Assche]

On 10/29/22 19:27, Jinlong Chen wrote:
>> I think this patch introduces a hang for every caller of
>> blk_mq_destroy_queue() other than blk_queue_start_drain().
 >> I don't see why the patch introduces a hang. The calling relationship in
> blk_mq_destroy_queue is as follows: [ ... ]

Agreed - what I wrote is wrong.

> So I think there is a redundant call to blk_freeze_queue_start(), we
> just need to call blk_mq_freeze_queue_wait() after calling
> blk_queue_start_drain().

I think it is on purpose that blk_queue_start_drain() freezes the 
request queue and never unfreezes it. So if you want to change this 
behavior it's up to you to motivate why you want to change this behavior 
and also why it is safe to make that change. See also commit 
d3cfb2a0ac0b ("block: block new I/O just after queue is set as dying").

Bart.