[PATCH 0/5] block: freeze/unfreeze lockdep fixes

[PATCH 0/5] block: freeze/unfreeze lockdep fixes

[Ming Lei]

Hello,

The 1st patch removes blk_freeze_queue().

The 2nd & 3rd patches add blk_mq_freeze_queue_non_owner() and apply
it on rbd.

The 4th patches fixes potential unfreeze lock verification on non-owner
context.

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

Ming Lei (5):
  block: remove blk_freeze_queue()
  blk-mq: add non_owner variant of blk_mq_freeze_queue API
  rbd: convert to blk_mq_freeze_queue_non_owner
  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         | 98 ++++++++++++++++++++++++++++--------------
 block/blk.h            |  4 +-
 block/elevator.c       |  8 +++-
 drivers/block/rbd.c    |  2 +-
 include/linux/blk-mq.h |  1 +
 include/linux/blkdev.h |  4 ++
 7 files changed, 81 insertions(+), 38 deletions(-)

-- 
2.47.0

[PATCH 1/5] block: remove blk_freeze_queue()

[Ming Lei]

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

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

Re: [PATCH 1/5] block: remove blk_freeze_queue()

[Christoph Hellwig]

At some point we should sort out the mq vs non-mq naming, which is
a bit nasty becuase the freeze functions work for non-mq disks, but
do so slightly differently..

For now this looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/5] blk-mq: add non_owner variant of blk_mq_freeze_queue API

[Ming Lei]

Add non_owner variant of blk_mq_freeze_queue API for rbd.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c         | 14 +++++++++++---
 include/linux/blk-mq.h |  1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 5f4496220432..8e18284ede8f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -195,12 +195,20 @@ void blk_mq_unfreeze_queue(struct request_queue *q)
 EXPORT_SYMBOL_GPL(blk_mq_unfreeze_queue);
 
 /*
- * non_owner variant of blk_freeze_queue_start
+ * non_owner variant of blk_mq_freeze_queue
  *
- * Unlike blk_freeze_queue_start, the queue doesn't need to be unfrozen
- * by the same task.  This is fragile and should not be used if at all
+ * Unlike blk_mq_freeze_queue, the queue doesn't need to be unfrozen by
+ * the same task. This is fragile and should not be used if at all
  * possible.
  */
+void blk_mq_freeze_queue_non_owner(struct request_queue *q)
+{
+	__blk_freeze_queue_start(q);
+	blk_mq_freeze_queue_wait(q);
+}
+EXPORT_SYMBOL_GPL(blk_mq_freeze_queue_non_owner);
+
+/* non_owner variant of blk_freeze_queue_start */
 void blk_freeze_queue_start_non_owner(struct request_queue *q)
 {
 	__blk_freeze_queue_start(q);
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 2035fad3131f..ed15dc2e7bd6 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -919,6 +919,7 @@ void blk_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);
+void blk_mq_freeze_queue_non_owner(struct request_queue *q);
 void blk_mq_unfreeze_queue_non_owner(struct request_queue *q);
 void blk_freeze_queue_start_non_owner(struct request_queue *q);
 
-- 
2.47.0

[PATCH 3/5] rbd: convert to blk_mq_freeze_queue_non_owner

[Ming Lei]

rbd just calls blk_mq_freeze_queue() only, and doesn't unfreeze queue in
current context, so convert to blk_mq_freeze_queue_non_owner().

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 9c8b19a22c2a..63c183ecdad9 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -7282,7 +7282,7 @@ static ssize_t do_rbd_remove(const char *buf, size_t count)
 		 * Prevent new IO from being queued and wait for existing
 		 * IO to complete/fail.
 		 */
-		blk_mq_freeze_queue(rbd_dev->disk->queue);
+		blk_mq_freeze_queue_non_owner(rbd_dev->disk->queue);
 		blk_mark_disk_dead(rbd_dev->disk);
 	}
 
-- 
2.47.0

Re: [PATCH 3/5] rbd: convert to blk_mq_freeze_queue_non_owner

[Christoph Hellwig]

On Wed, Oct 30, 2024 at 08:42:35PM +0800, Ming Lei wrote:
> rbd just calls blk_mq_freeze_queue() only, and doesn't unfreeze queue in
> current context, so convert to blk_mq_freeze_queue_non_owner().

I think the right fix here is to unfreeze after marking the disk
dead.

Re: [PATCH 3/5] rbd: convert to blk_mq_freeze_queue_non_owner

[Ming Lei]

On Wed, Oct 30, 2024 at 03:44:18PM +0100, Christoph Hellwig wrote:
> On Wed, Oct 30, 2024 at 08:42:35PM +0800, Ming Lei wrote:
> > rbd just calls blk_mq_freeze_queue() only, and doesn't unfreeze queue in
> > current context, so convert to blk_mq_freeze_queue_non_owner().
> 
> I think the right fix here is to unfreeze after marking the disk
> dead.

Looks fine.


Thanks, 
Ming

[PATCH 4/5] 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 freeze lock is still verified on the outmost one.

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         | 64 ++++++++++++++++++++++++++++++++++++------
 block/blk.h            |  3 +-
 include/linux/blkdev.h |  4 +++
 4 files changed, 62 insertions(+), 11 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 8e18284ede8f..0ec3b2db1d00 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_mq_freeze_queue_non_owner(struct request_queue *q)
 {
-	__blk_freeze_queue_start(q);
+	__blk_freeze_queue_start(q, NULL);
 	blk_mq_freeze_queue_wait(q);
 }
 EXPORT_SYMBOL_GPL(blk_mq_freeze_queue_non_owner);
@@ -211,7 +257,7 @@ EXPORT_SYMBOL_GPL(blk_mq_freeze_queue_non_owner);
 /* non_owner variant of blk_freeze_queue_start */
 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

Re: [PATCH 4/5] block: always verify unfreeze lock on the owner task

[Christoph Hellwig]

On Wed, Oct 30, 2024 at 08:42:36PM +0800, Ming Lei wrote:
> 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.

Well, that's how non-owner functions work in general.

> Fix this issue by always verifying the last unfreeze lock on the owner
> task context, and freeze lock is still verified on the outmost one.
> 
> Fixes: f1be1788a32e ("block: model freeze & enter queue as lock for supporting lockdep")

What does this actually fix vs just improving coverage?  Because the
hacks in here look pretty horrible and I'd be much happier if we didn't
have them.

Re: [PATCH 4/5] block: always verify unfreeze lock on the owner task

[Ming Lei]

On Wed, Oct 30, 2024 at 03:46:14PM +0100, Christoph Hellwig wrote:
> On Wed, Oct 30, 2024 at 08:42:36PM +0800, Ming Lei wrote:
> > 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.
> 
> Well, that's how non-owner functions work in general.
> 
> > Fix this issue by always verifying the last unfreeze lock on the owner
> > task context, and freeze lock is still verified on the outmost one.
> > 
> > Fixes: f1be1788a32e ("block: model freeze & enter queue as lock for supporting lockdep")
> 
> What does this actually fix vs just improving coverage?  Because the
> hacks in here look pretty horrible and I'd be much happier if we didn't
> have them.
 
task A								task B

blk_mq_freeze_queue()
									blk_mq_freeze_queue()

blk_mq_unfreeze_queue()
									blk_mq_unfreeze_queue()


freeze_queue is verified on task A, but unfreeze_queue is verified on
task B, this way is definitely wrong.

This patch moves unfreeze_queue verification on task A.


Thanks, 
Ming

[PATCH 5/5] 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 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index f169f4bae917..a02bf911d3ca 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -598,13 +598,17 @@ 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_mq_freeze_queue_non_owner(q);
+	blk_freeze_acquire_lock(q, true, false);
 	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 5/5] block: don't verify IO lock for freeze/unfreeze in elevator_init_mq()

[Christoph Hellwig]

On Wed, Oct 30, 2024 at 08:42:37PM +0800, Ming Lei wrote:
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -598,13 +598,17 @@ 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_mq_freeze_queue_non_owner(q);
> +	blk_freeze_acquire_lock(q, true, false);
>  	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);

Why do we need to free at all from the add_disk case?  The passthrough
command should never hit the elevator, or am I missing something?

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

[Ming Lei]

On Wed, Oct 30, 2024 at 03:46:52PM +0100, Christoph Hellwig wrote:
> On Wed, Oct 30, 2024 at 08:42:37PM +0800, Ming Lei wrote:
> > --- a/block/elevator.c
> > +++ b/block/elevator.c
> > @@ -598,13 +598,17 @@ 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_mq_freeze_queue_non_owner(q);
> > +	blk_freeze_acquire_lock(q, true, false);
> >  	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);
> 
> Why do we need to free at all from the add_disk case?  The passthrough
> command should never hit the elevator, or am I missing something?

In theory the queue needn't to be frozen here, but both FS IO and PT req
share common blk-mq code, in which q->elevator is often referenced.


Thanks, 
Ming