[PATCH V2 0/4] block: freeze/unfreeze lockdep fixes

[PATCH V2 0/4] block: freeze/unfreeze lockdep fixes

[Ming Lei]

Hello,

The 1st patch removes blk_freeze_queue().

The 2nd patch fixes freeze uses in rbd.

The 3rd patches fixes potential unfreeze lock verification on non-owner
context.

The 4th patch doesn't verify io lock in elevator_init_mq() for fixing
false lockdep warning.

V2:
	- drop patch 1 and fix rbd by adding unfreeze (Christoph)
	- add reviewed-by


Ming Lei (4):
  block: remove blk_freeze_queue()
  rbd: unfreeze queue after marking disk as dead
  block: always verify unfreeze lock on the owner task
  block: don't verify IO lock for freeze/unfreeze in elevator_init_mq()

 block/blk-core.c       |  2 +-
 block/blk-mq.c         | 84 +++++++++++++++++++++++++++---------------
 block/blk.h            |  4 +-
 block/elevator.c       | 10 ++++-
 drivers/block/rbd.c    |  1 +
 include/linux/blkdev.h |  4 ++
 6 files changed, 71 insertions(+), 34 deletions(-)

-- 
2.47.0

[PATCH V2 1/4] block: remove blk_freeze_queue()

[Ming Lei]

No one use blk_freeze_queue(), so remove it and the obsolete comment.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 22 +---------------------
 block/blk.h    |  1 -
 2 files changed, 1 insertion(+), 22 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4ae7eb335fbd..5f4496220432 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -161,31 +161,11 @@ int blk_mq_freeze_queue_wait_timeout(struct request_queue *q,
 }
 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);
 
 bool __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic)
diff --git a/block/blk.h b/block/blk.h
index 63d5df0dc29c..ac48b79cbf80 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -35,7 +35,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);
 bool __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic);
 bool blk_queue_start_drain(struct request_queue *q);
 bool __blk_freeze_queue_start(struct request_queue *q);
-- 
2.47.0

[PATCH V2 2/4] rbd: unfreeze queue after marking disk as dead

[Ming Lei]

Unfreeze queue after returning from blk_mark_disk_dead(), this way at
least allows us to verify queue freeze correctly with lockdep.

Suggested-by: Christoph Hellwig <hch@lst.de>
Cc: Ilya Dryomov <idryomov@gmail.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/rbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 9c8b19a22c2a..ac421dbeeb11 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -7284,6 +7284,7 @@ static ssize_t do_rbd_remove(const char *buf, size_t count)
 		 */
 		blk_mq_freeze_queue(rbd_dev->disk->queue);
 		blk_mark_disk_dead(rbd_dev->disk);
+		blk_mq_unfreeze_queue(rbd_dev->disk->queue);
 	}
 
 	del_gendisk(rbd_dev->disk);
-- 
2.47.0

[PATCH V2 3/4] block: always verify unfreeze lock on the owner task

[Ming Lei]

commit f1be1788a32e ("block: model freeze & enter queue as lock for
supporting lockdep") tries to apply lockdep for verifying freeze &
unfreeze. However, the verification is only done the outmost freeze and
unfreeze. This way is actually not correct because q->mq_freeze_depth
still may drop to zero on other task instead of the freeze owner task.

Fix this issue by always verifying the last unfreeze lock on the owner
task context, and make sure both the outmost freeze & unfreeze are
verified in the current task.

Fixes: f1be1788a32e ("block: model freeze & enter queue as lock for supporting lockdep")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-core.c       |  2 +-
 block/blk-mq.c         | 62 ++++++++++++++++++++++++++++++++++++------
 block/blk.h            |  3 +-
 include/linux/blkdev.h |  4 +++
 4 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 09d10bb95fda..4f791a3114a1 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -287,7 +287,7 @@ bool blk_queue_start_drain(struct request_queue *q)
 	 * entering queue, so we call blk_freeze_queue_start() to
 	 * prevent I/O from crossing blk_queue_enter().
 	 */
-	bool freeze = __blk_freeze_queue_start(q);
+	bool freeze = __blk_freeze_queue_start(q, current);
 	if (queue_is_mq(q))
 		blk_mq_wake_waiters(q);
 	/* Make blk_queue_enter() reexamine the DYING flag. */
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 5f4496220432..5e240a4b6be0 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -120,20 +120,66 @@ void blk_mq_in_flight_rw(struct request_queue *q, struct block_device *part,
 	inflight[1] = mi.inflight[1];
 }
 
-bool __blk_freeze_queue_start(struct request_queue *q)
+#ifdef CONFIG_LOCKDEP
+static bool blk_freeze_set_owner(struct request_queue *q,
+				 struct task_struct *owner)
 {
-	int freeze;
+	if (!owner)
+		return false;
+
+	if (!q->mq_freeze_depth) {
+		q->mq_freeze_owner = owner;
+		q->mq_freeze_owner_depth = 1;
+		return true;
+	}
+
+	if (owner == q->mq_freeze_owner)
+		q->mq_freeze_owner_depth += 1;
+	return false;
+}
+
+/* verify the last unfreeze in owner context */
+static bool blk_unfreeze_check_owner(struct request_queue *q)
+{
+	if (!q->mq_freeze_owner)
+		return false;
+	if (q->mq_freeze_owner != current)
+		return false;
+	if (--q->mq_freeze_owner_depth == 0) {
+		q->mq_freeze_owner = NULL;
+		return true;
+	}
+	return false;
+}
+
+#else
+
+static bool blk_freeze_set_owner(struct request_queue *q,
+				 struct task_struct *owner)
+{
+	return false;
+}
+
+static bool blk_unfreeze_check_owner(struct request_queue *q)
+{
+	return false;
+}
+#endif
+
+bool __blk_freeze_queue_start(struct request_queue *q,
+			      struct task_struct *owner)
+{
+	bool freeze;
 
 	mutex_lock(&q->mq_freeze_lock);
+	freeze = blk_freeze_set_owner(q, owner);
 	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);
-		freeze = true;
 	} else {
 		mutex_unlock(&q->mq_freeze_lock);
-		freeze = false;
 	}
 
 	return freeze;
@@ -141,7 +187,7 @@ bool __blk_freeze_queue_start(struct request_queue *q)
 
 void blk_freeze_queue_start(struct request_queue *q)
 {
-	if (__blk_freeze_queue_start(q))
+	if (__blk_freeze_queue_start(q, current))
 		blk_freeze_acquire_lock(q, false, false);
 }
 EXPORT_SYMBOL_GPL(blk_freeze_queue_start);
@@ -170,7 +216,7 @@ EXPORT_SYMBOL_GPL(blk_mq_freeze_queue);
 
 bool __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic)
 {
-	int unfreeze = false;
+	bool unfreeze;
 
 	mutex_lock(&q->mq_freeze_lock);
 	if (force_atomic)
@@ -180,8 +226,8 @@ bool __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic)
 	if (!q->mq_freeze_depth) {
 		percpu_ref_resurrect(&q->q_usage_counter);
 		wake_up_all(&q->mq_freeze_wq);
-		unfreeze = true;
 	}
+	unfreeze = blk_unfreeze_check_owner(q);
 	mutex_unlock(&q->mq_freeze_lock);
 
 	return unfreeze;
@@ -203,7 +249,7 @@ EXPORT_SYMBOL_GPL(blk_mq_unfreeze_queue);
  */
 void blk_freeze_queue_start_non_owner(struct request_queue *q)
 {
-	__blk_freeze_queue_start(q);
+	__blk_freeze_queue_start(q, NULL);
 }
 EXPORT_SYMBOL_GPL(blk_freeze_queue_start_non_owner);
 
diff --git a/block/blk.h b/block/blk.h
index ac48b79cbf80..57fc035620d6 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -37,7 +37,8 @@ void blk_free_flush_queue(struct blk_flush_queue *q);
 
 bool __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic);
 bool blk_queue_start_drain(struct request_queue *q);
-bool __blk_freeze_queue_start(struct request_queue *q);
+bool __blk_freeze_queue_start(struct request_queue *q,
+			      struct task_struct *owner);
 int __bio_queue_enter(struct request_queue *q, struct bio *bio);
 void submit_bio_noacct_nocheck(struct bio *bio);
 void bio_await_chain(struct bio *bio);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 7bfc877e159e..379cd8eebdd9 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -575,6 +575,10 @@ struct request_queue {
 	struct throtl_data *td;
 #endif
 	struct rcu_head		rcu_head;
+#ifdef CONFIG_LOCKDEP
+	struct task_struct	*mq_freeze_owner;
+	int			mq_freeze_owner_depth;
+#endif
 	wait_queue_head_t	mq_freeze_wq;
 	/*
 	 * Protect concurrent access to q_usage_counter by
-- 
2.47.0

[PATCH V2 4/4] block: don't verify IO lock for freeze/unfreeze in elevator_init_mq()

[Ming Lei]

elevator_init_mq() is only called at the entry of add_disk_fwnode() when
disk IO isn't allowed yet.

So not verify io lock(q->io_lockdep_map) for freeze & unfreeze in
elevator_init_mq().

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reported-by: Lai Yi <yi1.lai@linux.intel.com>
Fixes: f1be1788a32e ("block: model freeze & enter queue as lock for supporting lockdep")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/elevator.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index f169f4bae917..7c3ba80e5ff4 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -598,13 +598,19 @@ void elevator_init_mq(struct request_queue *q)
 	 * drain any dispatch activities originated from passthrough
 	 * requests, then no need to quiesce queue which may add long boot
 	 * latency, especially when lots of disks are involved.
+	 *
+	 * Disk isn't added yet, so verifying queue lock only manually.
 	 */
-	blk_mq_freeze_queue(q);
+	blk_freeze_queue_start_non_owner(q);
+	blk_freeze_acquire_lock(q, true, false);
+	blk_mq_freeze_queue_wait(q);
+
 	blk_mq_cancel_work_sync(q);
 
 	err = blk_mq_init_sched(q, e);
 
-	blk_mq_unfreeze_queue(q);
+	blk_unfreeze_release_lock(q, true, false);
+	blk_mq_unfreeze_queue_non_owner(q);
 
 	if (err) {
 		pr_warn("\"%s\" elevator initialization failed, "
-- 
2.47.0

Re: [PATCH V2 0/4] block: freeze/unfreeze lockdep fixes

[Ming Lei]

On Thu, Oct 31, 2024 at 09:37:16PM +0800, Ming Lei wrote:
> Hello,
> 
> The 1st patch removes blk_freeze_queue().
> 
> The 2nd patch fixes freeze uses in rbd.
> 
> The 3rd patches fixes potential unfreeze lock verification on non-owner
> context.
> 
> The 4th patch doesn't verify io lock in elevator_init_mq() for fixing
> false lockdep warning.
> 
> V2:
> 	- drop patch 1 and fix rbd by adding unfreeze (Christoph)
> 	- add reviewed-by

Hi Jens,

This patchset fixes several lockdep false positive warnings, can you
merge them if you are fine?

https://lore.kernel.org/linux-block/Zym_h_EYRVX18dSw@fedora/



thanks,
Ming

Re: [PATCH V2 0/4] block: freeze/unfreeze lockdep fixes

[Ming Lei]

On Wed, Nov 6, 2024 at 8:24 AM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Thu, Oct 31, 2024 at 09:37:16PM +0800, Ming Lei wrote:
> > Hello,
> >
> > The 1st patch removes blk_freeze_queue().
> >
> > The 2nd patch fixes freeze uses in rbd.
> >
> > The 3rd patches fixes potential unfreeze lock verification on non-owner
> > context.
> >
> > The 4th patch doesn't verify io lock in elevator_init_mq() for fixing
> > false lockdep warning.
> >
> > V2:
> >       - drop patch 1 and fix rbd by adding unfreeze (Christoph)
> >       - add reviewed-by
>
> Hi Jens,
>
> This patchset fixes several lockdep false positive warnings, can you
> merge them if you are fine?
>
> https://lore.kernel.org/linux-block/Zym_h_EYRVX18dSw@fedora/

Hi Jens,

There are lots of lockdep reports which need the fixes, so ping...

Thanks,

Re: [PATCH V2 0/4] block: freeze/unfreeze lockdep fixes

[Jens Axboe]

On 11/7/24 4:21 PM, Ming Lei wrote:
> On Wed, Nov 6, 2024 at 8:24?AM Ming Lei <ming.lei@redhat.com> wrote:
>>
>> On Thu, Oct 31, 2024 at 09:37:16PM +0800, Ming Lei wrote:
>>> Hello,
>>>
>>> The 1st patch removes blk_freeze_queue().
>>>
>>> The 2nd patch fixes freeze uses in rbd.
>>>
>>> The 3rd patches fixes potential unfreeze lock verification on non-owner
>>> context.
>>>
>>> The 4th patch doesn't verify io lock in elevator_init_mq() for fixing
>>> false lockdep warning.
>>>
>>> V2:
>>>       - drop patch 1 and fix rbd by adding unfreeze (Christoph)
>>>       - add reviewed-by
>>
>> Hi Jens,
>>
>> This patchset fixes several lockdep false positive warnings, can you
>> merge them if you are fine?
>>
>> https://lore.kernel.org/linux-block/Zym_h_EYRVX18dSw@fedora/
> 
> Hi Jens,
> 
> There are lots of lockdep reports which need the fixes, so ping...

Oops indeed, not sure why I missed this one.

-- 
Jens Axboe

Re: [PATCH V2 0/4] block: freeze/unfreeze lockdep fixes

[Jens Axboe]

On Thu, 31 Oct 2024 21:37:16 +0800, Ming Lei wrote:
> The 1st patch removes blk_freeze_queue().
> 
> The 2nd patch fixes freeze uses in rbd.
> 
> The 3rd patches fixes potential unfreeze lock verification on non-owner
> context.
> 
> [...]

Applied, thanks!

[1/4] block: remove blk_freeze_queue()
      commit: 54027869df83aceccd18c4a799b263a2b318b065
[2/4] rbd: unfreeze queue after marking disk as dead
      commit: a471977780cc8ba809f84e3e2289d676063e7547
[3/4] block: always verify unfreeze lock on the owner task
      commit: 6a78699838a0ddeed3620ddf50c1521f1fe1e811
[4/4] block: don't verify IO lock for freeze/unfreeze in elevator_init_mq()
      commit: 357e1b7f730bd85a383e7afa75a3caba329c5707

Best regards,
-- 
Jens Axboe