[PATCH] blk-mq: fix WARNING "percpu_ref_kill() called more than once!"

[PATCH] blk-mq: fix WARNING "percpu_ref_kill() called more than once!"

[Ming Lei]

Before doing queue release, the queue has been freezed already
by blk_cleanup_queue(), so needn't to freeze queue for deleting
tag set.

This patch fixes the WARNING of "percpu_ref_kill() called more than once!"
which is triggered during unloading block driver.

Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 block/blk-mq.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 5189cb1..ac8a041 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1713,14 +1713,10 @@ static void blk_mq_del_queue_tag_set(struct request_queue *q)
 {
 	struct blk_mq_tag_set *set = q->tag_set;
 
-	blk_mq_freeze_queue(q);
-
 	mutex_lock(&set->tag_list_lock);
 	list_del_init(&q->tag_set_list);
 	blk_mq_update_tag_set_depth(set);
 	mutex_unlock(&set->tag_list_lock);
-
-	blk_mq_unfreeze_queue(q);
 }
 
 static void blk_mq_add_queue_tag_set(struct blk_mq_tag_set *set,
-- 
1.7.9.5

Re: [PATCH] blk-mq: fix WARNING "percpu_ref_kill() called more than once!"

[Jens Axboe]

On 2014-08-15 09:16, Ming Lei wrote:
> Before doing queue release, the queue has been freezed already
> by blk_cleanup_queue(), so needn't to freeze queue for deleting
> tag set.
>
> This patch fixes the WARNING of "percpu_ref_kill() called more than once!"
> which is triggered during unloading block driver.

Thanks, you are right, we need not call it there. I'll apply this for 3.17.

-- 
Jens Axboe

[PATCH] blk-mq: blk_mq_freeze_queue() should allow nesting

[Tejun Heo]

While converting to percpu_ref for freezing, add703fda981 ("blk-mq:
use percpu_ref for mq usage count") incorrectly made
blk_mq_freeze_queue() misbehave when freezing is nested due to
percpu_ref_kill() being invoked on an already killed ref.

Fix it by making blk_mq_freeze_queue() kill and kick the queue only
for the outermost freeze attempt.  All the nested ones can simply wait
for the ref to reach zero.

While at it, remove unnecessary @wake initialization from
blk_mq_unfreeze_queue().

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Ming Lei <ming.lei@canonical.com>
---
Hello, Ming, Jens.

I think something like this is the correct solution.  Freezing should
be nestable after all.  Ming, can you please verify whether this fixes
the issue you're seeing?

Thanks.

 block/blk-mq.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 5189cb1..5978088 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -112,18 +112,22 @@ static void blk_mq_usage_counter_release(struct percpu_ref *ref)
  */
 void blk_mq_freeze_queue(struct request_queue *q)
 {
+	bool freeze;
+
 	spin_lock_irq(q->queue_lock);
-	q->mq_freeze_depth++;
+	freeze = !q->mq_freeze_depth++;
 	spin_unlock_irq(q->queue_lock);
 
-	percpu_ref_kill(&q->mq_usage_counter);
-	blk_mq_run_queues(q, false);
+	if (freeze) {
+		percpu_ref_kill(&q->mq_usage_counter);
+		blk_mq_run_queues(q, false);
+	}
 	wait_event(q->mq_freeze_wq, percpu_ref_is_zero(&q->mq_usage_counter));
 }
 
 static void blk_mq_unfreeze_queue(struct request_queue *q)
 {
-	bool wake = false;
+	bool wake;
 
 	spin_lock_irq(q->queue_lock);
 	wake = !--q->mq_freeze_depth;

Re: [PATCH] blk-mq: blk_mq_freeze_queue() should allow nesting

[Ming Lei]

On 8/16/14, Tejun Heo <tj@kernel.org> wrote:
> While converting to percpu_ref for freezing, add703fda981 ("blk-mq:
> use percpu_ref for mq usage count") incorrectly made
> blk_mq_freeze_queue() misbehave when freezing is nested due to
> percpu_ref_kill() being invoked on an already killed ref.
>
> Fix it by making blk_mq_freeze_queue() kill and kick the queue only
> for the outermost freeze attempt.  All the nested ones can simply wait
> for the ref to reach zero.
>
> While at it, remove unnecessary @wake initialization from
> blk_mq_unfreeze_queue().
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Ming Lei <ming.lei@canonical.com>
> ---
> Hello, Ming, Jens.
>
> I think something like this is the correct solution.  Freezing should
> be nestable after all.  Ming, can you please verify whether this fixes
> the issue you're seeing?

This patch can fix the warning too, and I think freezing should be nestable,
and in my another patch they are exported.

Thanks,

> Thanks.
>
>  block/blk-mq.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 5189cb1..5978088 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -112,18 +112,22 @@ static void blk_mq_usage_counter_release(struct
> percpu_ref *ref)
>   */
>  void blk_mq_freeze_queue(struct request_queue *q)
>  {
> +	bool freeze;
> +
>  	spin_lock_irq(q->queue_lock);
> -	q->mq_freeze_depth++;
> +	freeze = !q->mq_freeze_depth++;
>  	spin_unlock_irq(q->queue_lock);
>
> -	percpu_ref_kill(&q->mq_usage_counter);
> -	blk_mq_run_queues(q, false);
> +	if (freeze) {
> +		percpu_ref_kill(&q->mq_usage_counter);
> +		blk_mq_run_queues(q, false);
> +	}
>  	wait_event(q->mq_freeze_wq, percpu_ref_is_zero(&q->mq_usage_counter));
>  }
>
>  static void blk_mq_unfreeze_queue(struct request_queue *q)
>  {
> -	bool wake = false;
> +	bool wake;
>
>  	spin_lock_irq(q->queue_lock);
>  	wake = !--q->mq_freeze_depth;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>


-- 
Ming Lei

Re: [PATCH] blk-mq: blk_mq_freeze_queue() should allow nesting

[Jens Axboe]

On 2014-08-16 06:02, Tejun Heo wrote:
> While converting to percpu_ref for freezing, add703fda981 ("blk-mq:
> use percpu_ref for mq usage count") incorrectly made
> blk_mq_freeze_queue() misbehave when freezing is nested due to
> percpu_ref_kill() being invoked on an already killed ref.
>
> Fix it by making blk_mq_freeze_queue() kill and kick the queue only
> for the outermost freeze attempt.  All the nested ones can simply wait
> for the ref to reach zero.
>
> While at it, remove unnecessary @wake initialization from
> blk_mq_unfreeze_queue().
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Ming Lei <ming.lei@canonical.com>
> ---
> Hello, Ming, Jens.
>
> I think something like this is the correct solution.  Freezing should
> be nestable after all.  Ming, can you please verify whether this fixes
> the issue you're seeing?

I had completely missed that nesting no longer works, which it of course 
should. Mings patch is still fine, however, as we don't need the freeze 
at that point. But I'll apply yours as well so that nesting is restored, 
it might be important for other cases. And it's one of those things that 
we should not have to worry about.

-- 
Jens Axboe