[PATCH 0/3] block: fix lock dependency between freeze and elevator lock

[PATCH 0/3] block: fix lock dependency between freeze and elevator lock

[Ming Lei]

Hello Jens,

This patchset adds two pair of block internal APIs for addressing recent
lockdep report between freeze and elevator lock.

Thanks,
Ming


Ming Lei (3):
  block: add blk_mq_enter_no_io() and blk_mq_exit_no_io()
  block: don't call freeze queue in elevator_switch() and
    elevator_disable()
  block: use blk_mq_no_io() for avoiding lock dependency

 block/blk-core.c       |  6 ++++--
 block/blk-mq.c         | 25 ++++++++++++++++++-------
 block/blk-mq.h         | 19 +++++++++++++++++++
 block/blk-sysfs.c      |  8 ++++----
 block/blk.h            |  5 +++--
 block/elevator.c       | 11 ++---------
 include/linux/blkdev.h |  8 ++++++++
 7 files changed, 58 insertions(+), 24 deletions(-)

-- 
2.47.0

[PATCH 1/3] block: add blk_mq_enter_no_io() and blk_mq_exit_no_io()

[Ming Lei]

Add blk_mq_enter_no_io() and blk_mq_exit_no_io() for preventing queue
from handling any FS or passthrough IO, meantime the queue is kept in
non-freeze state.

The added two APIs are for avoiding many potential lock risk related
with freeze lock.

Also add two variants of memsave version.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-core.c       |  6 ++++--
 block/blk-mq.c         | 18 ++++++++++++++++--
 block/blk-mq.h         | 19 +++++++++++++++++++
 block/blk.h            |  5 +++--
 include/linux/blkdev.h |  8 ++++++++
 5 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 4623de79effa..a54a18fada8a 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -319,7 +319,8 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
 		smp_rmb();
 		wait_event(q->mq_freeze_wq,
 			   (!q->mq_freeze_depth &&
-			    blk_pm_resume_queue(pm, q)) ||
+			    (blk_pm_resume_queue(pm, q) ||
+			     !blk_queue_no_io(q))) ||
 			   blk_queue_dying(q));
 		if (blk_queue_dying(q))
 			return -ENODEV;
@@ -352,7 +353,8 @@ int __bio_queue_enter(struct request_queue *q, struct bio *bio)
 		smp_rmb();
 		wait_event(q->mq_freeze_wq,
 			   (!q->mq_freeze_depth &&
-			    blk_pm_resume_queue(false, q)) ||
+			    (blk_pm_resume_queue(false, q) ||
+			     !blk_queue_no_io(q))) ||
 			   test_bit(GD_DEAD, &disk->state));
 		if (test_bit(GD_DEAD, &disk->state))
 			goto dead;
diff --git a/block/blk-mq.c b/block/blk-mq.c
index ae8494d88897..075ee51066b3 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -222,8 +222,7 @@ bool __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic)
 	bool unfreeze;
 
 	mutex_lock(&q->mq_freeze_lock);
-	if (force_atomic)
-		q->q_usage_counter.data->force_atomic = true;
+	q->q_usage_counter.data->force_atomic = force_atomic;
 	q->mq_freeze_depth--;
 	WARN_ON_ONCE(q->mq_freeze_depth < 0);
 	if (!q->mq_freeze_depth) {
@@ -278,6 +277,21 @@ void blk_mq_quiesce_queue_nowait(struct request_queue *q)
 }
 EXPORT_SYMBOL_GPL(blk_mq_quiesce_queue_nowait);
 
+void blk_mq_enter_no_io(struct request_queue *q)
+{
+	blk_mq_freeze_queue_nomemsave(q);
+	q->no_io = true;
+	if (__blk_mq_unfreeze_queue(q, true))
+		blk_unfreeze_release_lock(q);
+}
+
+void blk_mq_exit_no_io(struct request_queue *q)
+{
+	blk_mq_freeze_queue_nomemsave(q);
+	q->no_io = false;
+	blk_mq_unfreeze_queue_nomemrestore(q);
+}
+
 /**
  * blk_mq_wait_quiesce_done() - wait until in-progress quiesce is done
  * @set: tag_set to wait on
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 3011a78cf16a..f49070c8c05f 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -452,4 +452,23 @@ static inline bool blk_mq_can_poll(struct request_queue *q)
 		q->tag_set->map[HCTX_TYPE_POLL].nr_queues;
 }
 
+void blk_mq_enter_no_io(struct request_queue *q);
+void blk_mq_exit_no_io(struct request_queue *q);
+
+static inline unsigned int __must_check
+blk_mq_enter_no_io_memsave(struct request_queue *q)
+{
+	unsigned int memflags = memalloc_noio_save();
+
+	blk_mq_enter_no_io(q);
+	return memflags;
+}
+
+static inline void
+blk_mq_exit_no_io_memrestore(struct request_queue *q, unsigned int memflags)
+{
+	blk_mq_exit_no_io(q);
+	memalloc_noio_restore(memflags);
+}
+
 #endif
diff --git a/block/blk.h b/block/blk.h
index 006e3be433d2..7d0994c1d3ad 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -56,8 +56,9 @@ static inline bool blk_try_enter_queue(struct request_queue *q, bool pm)
 	 * The code that increments the pm_only counter must ensure that the
 	 * counter is globally visible before the queue is unfrozen.
 	 */
-	if (blk_queue_pm_only(q) &&
-	    (!pm || queue_rpm_status(q) == RPM_SUSPENDED))
+	if ((blk_queue_pm_only(q) &&
+	    (!pm || queue_rpm_status(q) == RPM_SUSPENDED)) ||
+			blk_queue_no_io(q))
 		goto fail_put;
 
 	rcu_read_unlock();
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index e39c45bc0a97..1b8fd63eee80 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -498,6 +498,13 @@ struct request_queue {
 
 	int			quiesce_depth;
 
+	/*
+	 * Prevent queue from handling IO
+	 *
+	 * keep it in same cache line with q_usage_counter
+	 */
+	bool			no_io;
+
 	struct gendisk		*disk;
 
 	/*
@@ -679,6 +686,7 @@ void blk_queue_flag_clear(unsigned int flag, struct request_queue *q);
 #define blk_queue_sq_sched(q)	test_bit(QUEUE_FLAG_SQ_SCHED, &(q)->queue_flags)
 #define blk_queue_skip_tagset_quiesce(q) \
 	((q)->limits.features & BLK_FEAT_SKIP_TAGSET_QUIESCE)
+#define blk_queue_no_io(q)	(q->no_io)
 
 extern void blk_set_pm_only(struct request_queue *q);
 extern void blk_clear_pm_only(struct request_queue *q);
-- 
2.47.0

[PATCH 2/3] block: don't call freeze queue in elevator_switch() and elevator_disable()

[Ming Lei]

Both elevator_switch() and elevator_disable() are called from sysfs
store and updating nr_hw_queue code paths only.

And in the two code paths, queue has been frozen already, so don't call
freeze queue in the two functions.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/elevator.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index b4d08026b02c..4d3a8f996c91 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -615,12 +615,10 @@ void elevator_init_mq(struct request_queue *q)
  */
 int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
 {
-	unsigned int memflags;
 	int ret;
 
 	lockdep_assert_held(&q->elevator_lock);
 
-	memflags = blk_mq_freeze_queue(q);
 	blk_mq_quiesce_queue(q);
 
 	if (q->elevator) {
@@ -641,7 +639,6 @@ int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
 
 out_unfreeze:
 	blk_mq_unquiesce_queue(q);
-	blk_mq_unfreeze_queue(q, memflags);
 
 	if (ret) {
 		pr_warn("elv: switch to \"%s\" failed, falling back to \"none\"\n",
@@ -653,11 +650,8 @@ int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
 
 void elevator_disable(struct request_queue *q)
 {
-	unsigned int memflags;
-
 	lockdep_assert_held(&q->elevator_lock);
 
-	memflags = blk_mq_freeze_queue(q);
 	blk_mq_quiesce_queue(q);
 
 	elv_unregister_queue(q);
@@ -668,7 +662,6 @@ void elevator_disable(struct request_queue *q)
 	blk_add_trace_msg(q, "elv switch: none");
 
 	blk_mq_unquiesce_queue(q);
-	blk_mq_unfreeze_queue(q, memflags);
 }
 
 /*
-- 
2.47.0

[PATCH 3/3] block: use blk_mq_no_io() for avoiding lock dependency

[Ming Lei]

Use blk_mq_no_io() to prevent IO from entering queue for avoiding lock
dependency between freeze lock and elevator lock, and we have got many
such reports:

Reported-by: syzbot+4c7e0f9b94ad65811efb@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-block/67e6b425.050a0220.2f068f.007b.GAE@google.com/
Reported-by: Valdis Klētnieks <valdis.kletnieks@vt.edu>
Closes: https://lore.kernel.org/linux-block/7755.1743228130@turing-police/#t
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c    | 7 ++-----
 block/blk-sysfs.c | 8 ++++----
 block/elevator.c  | 4 ++--
 3 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 075ee51066b3..022d8139910d 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -4882,9 +4882,6 @@ int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr)
 	int ret;
 	unsigned long i;
 
-	if (WARN_ON_ONCE(!q->mq_freeze_depth))
-		return -EINVAL;
-
 	if (!set)
 		return -EINVAL;
 
@@ -5025,7 +5022,7 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 
 	memflags = memalloc_noio_save();
 	list_for_each_entry(q, &set->tag_list, tag_set_list)
-		blk_mq_freeze_queue_nomemsave(q);
+		blk_mq_enter_no_io(q);
 
 	/*
 	 * Switch IO scheduler to 'none', cleaning up the data associated
@@ -5074,7 +5071,7 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
 		blk_mq_elv_switch_back(&head, q);
 
 	list_for_each_entry(q, &set->tag_list, tag_set_list)
-		blk_mq_unfreeze_queue_nomemrestore(q);
+		blk_mq_exit_no_io(q);
 	memalloc_noio_restore(memflags);
 
 	/* Free the excess tags when nr_hw_queues shrink. */
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index a2882751f0d2..e866875c17be 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -76,7 +76,7 @@ queue_requests_store(struct gendisk *disk, const char *page, size_t count)
 	if (ret < 0)
 		return ret;
 
-	memflags = blk_mq_freeze_queue(q);
+	memflags = blk_mq_enter_no_io_memsave(q);
 	mutex_lock(&q->elevator_lock);
 	if (nr < BLKDEV_MIN_RQ)
 		nr = BLKDEV_MIN_RQ;
@@ -85,7 +85,7 @@ queue_requests_store(struct gendisk *disk, const char *page, size_t count)
 	if (err)
 		ret = err;
 	mutex_unlock(&q->elevator_lock);
-	blk_mq_unfreeze_queue(q, memflags);
+	blk_mq_exit_no_io_memrestore(q, memflags);
 	return ret;
 }
 
@@ -592,7 +592,7 @@ static ssize_t queue_wb_lat_store(struct gendisk *disk, const char *page,
 	if (val < -1)
 		return -EINVAL;
 
-	memflags = blk_mq_freeze_queue(q);
+	memflags = blk_mq_enter_no_io_memsave(q);
 	mutex_lock(&q->elevator_lock);
 
 	rqos = wbt_rq_qos(q);
@@ -623,7 +623,7 @@ static ssize_t queue_wb_lat_store(struct gendisk *disk, const char *page,
 	blk_mq_unquiesce_queue(q);
 out:
 	mutex_unlock(&q->elevator_lock);
-	blk_mq_unfreeze_queue(q, memflags);
+	blk_mq_exit_no_io_memrestore(q, memflags);
 
 	return ret;
 }
diff --git a/block/elevator.c b/block/elevator.c
index 4d3a8f996c91..c9cb8386bf5e 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -724,13 +724,13 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
 
 	elv_iosched_load_module(name);
 
-	memflags = blk_mq_freeze_queue(q);
+	memflags = blk_mq_enter_no_io_memsave(q);
 	mutex_lock(&q->elevator_lock);
 	ret = elevator_change(q, name);
 	if (!ret)
 		ret = count;
 	mutex_unlock(&q->elevator_lock);
-	blk_mq_unfreeze_queue(q, memflags);
+	blk_mq_exit_no_io_memrestore(q, memflags);
 	return ret;
 }
 
-- 
2.47.0

Re: [PATCH 1/3] block: add blk_mq_enter_no_io() and blk_mq_exit_no_io()

[Ming Lei]

On Wed, Apr 02, 2025 at 12:38:47PM +0800, Ming Lei wrote:
> Add blk_mq_enter_no_io() and blk_mq_exit_no_io() for preventing queue
> from handling any FS or passthrough IO, meantime the queue is kept in
> non-freeze state.
> 
> The added two APIs are for avoiding many potential lock risk related
> with freeze lock.
> 
> Also add two variants of memsave version.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-core.c       |  6 ++++--
>  block/blk-mq.c         | 18 ++++++++++++++++--
>  block/blk-mq.h         | 19 +++++++++++++++++++
>  block/blk.h            |  5 +++--
>  include/linux/blkdev.h |  8 ++++++++
>  5 files changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 4623de79effa..a54a18fada8a 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -319,7 +319,8 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
>  		smp_rmb();
>  		wait_event(q->mq_freeze_wq,
>  			   (!q->mq_freeze_depth &&
> -			    blk_pm_resume_queue(pm, q)) ||
> +			    (blk_pm_resume_queue(pm, q) ||
> +			     !blk_queue_no_io(q))) ||
>  			   blk_queue_dying(q));
>  		if (blk_queue_dying(q))
>  			return -ENODEV;
> @@ -352,7 +353,8 @@ int __bio_queue_enter(struct request_queue *q, struct bio *bio)
>  		smp_rmb();
>  		wait_event(q->mq_freeze_wq,
>  			   (!q->mq_freeze_depth &&
> -			    blk_pm_resume_queue(false, q)) ||
> +			    (blk_pm_resume_queue(false, q) ||

Here the above '||' should have been '&&'.

> +			     !blk_queue_no_io(q))) ||
>  			   test_bit(GD_DEAD, &disk->state));
>  		if (test_bit(GD_DEAD, &disk->state))
>  			goto dead;
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index ae8494d88897..075ee51066b3 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -222,8 +222,7 @@ bool __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic)
>  	bool unfreeze;
>  
>  	mutex_lock(&q->mq_freeze_lock);
> -	if (force_atomic)
> -		q->q_usage_counter.data->force_atomic = true;
> +	q->q_usage_counter.data->force_atomic = force_atomic;
>  	q->mq_freeze_depth--;
>  	WARN_ON_ONCE(q->mq_freeze_depth < 0);
>  	if (!q->mq_freeze_depth) {
> @@ -278,6 +277,21 @@ void blk_mq_quiesce_queue_nowait(struct request_queue *q)
>  }
>  EXPORT_SYMBOL_GPL(blk_mq_quiesce_queue_nowait);
>  
> +void blk_mq_enter_no_io(struct request_queue *q)
> +{
> +	blk_mq_freeze_queue_nomemsave(q);
> +	q->no_io = true;
> +	if (__blk_mq_unfreeze_queue(q, true))
> +		blk_unfreeze_release_lock(q);
> +}
> +
> +void blk_mq_exit_no_io(struct request_queue *q)
> +{
> +	blk_mq_freeze_queue_nomemsave(q);
> +	q->no_io = false;
> +	blk_mq_unfreeze_queue_nomemrestore(q);
> +}
> +
>  /**
>   * blk_mq_wait_quiesce_done() - wait until in-progress quiesce is done
>   * @set: tag_set to wait on
> diff --git a/block/blk-mq.h b/block/blk-mq.h
> index 3011a78cf16a..f49070c8c05f 100644
> --- a/block/blk-mq.h
> +++ b/block/blk-mq.h
> @@ -452,4 +452,23 @@ static inline bool blk_mq_can_poll(struct request_queue *q)
>  		q->tag_set->map[HCTX_TYPE_POLL].nr_queues;
>  }
>  
> +void blk_mq_enter_no_io(struct request_queue *q);
> +void blk_mq_exit_no_io(struct request_queue *q);
> +
> +static inline unsigned int __must_check
> +blk_mq_enter_no_io_memsave(struct request_queue *q)
> +{
> +	unsigned int memflags = memalloc_noio_save();
> +
> +	blk_mq_enter_no_io(q);
> +	return memflags;
> +}
> +
> +static inline void
> +blk_mq_exit_no_io_memrestore(struct request_queue *q, unsigned int memflags)
> +{
> +	blk_mq_exit_no_io(q);
> +	memalloc_noio_restore(memflags);
> +}
> +
>  #endif
> diff --git a/block/blk.h b/block/blk.h
> index 006e3be433d2..7d0994c1d3ad 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -56,8 +56,9 @@ static inline bool blk_try_enter_queue(struct request_queue *q, bool pm)
>  	 * The code that increments the pm_only counter must ensure that the
>  	 * counter is globally visible before the queue is unfrozen.
>  	 */
> -	if (blk_queue_pm_only(q) &&
> -	    (!pm || queue_rpm_status(q) == RPM_SUSPENDED))
> +	if ((blk_queue_pm_only(q) &&
> +	    (!pm || queue_rpm_status(q) == RPM_SUSPENDED)) ||

Same with above.


Thanks, 
Ming

Re: [PATCH 3/3] block: use blk_mq_no_io() for avoiding lock dependency

[Nilay Shroff]

On 4/2/25 10:08 AM, Ming Lei wrote:
> Use blk_mq_no_io() to prevent IO from entering queue for avoiding lock
> dependency between freeze lock and elevator lock, and we have got many
> such reports:
> 
> Reported-by: syzbot+4c7e0f9b94ad65811efb@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-block/67e6b425.050a0220.2f068f.007b.GAE@google.com/
> Reported-by: Valdis Klētnieks <valdis.kletnieks@vt.edu>
> Closes: https://lore.kernel.org/linux-block/7755.1743228130@turing-police/#t
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

I tested this series on my system and this works well as we cut dependency
between ->elevator_lock and ->freeze_lock. However don't we plan to now 
model blk_mq_enter_no_io and blk_mq_exit_no_io as lock/unlock for supporting 
lockdep? Maybe we don't. 

Overall changes looks good to me:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

Re: [PATCH 2/3] block: don't call freeze queue in elevator_switch() and elevator_disable()

[Nilay Shroff]

On 4/2/25 10:08 AM, Ming Lei wrote:
> Both elevator_switch() and elevator_disable() are called from sysfs
> store and updating nr_hw_queue code paths only.
> 
> And in the two code paths, queue has been frozen already, so don't call
> freeze queue in the two functions.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Looks good to me:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

Re: [PATCH 1/3] block: add blk_mq_enter_no_io() and blk_mq_exit_no_io()

[Nilay Shroff]

On 4/2/25 10:08 AM, Ming Lei wrote:
> Add blk_mq_enter_no_io() and blk_mq_exit_no_io() for preventing queue
> from handling any FS or passthrough IO, meantime the queue is kept in
> non-freeze state.
> 
> The added two APIs are for avoiding many potential lock risk related
> with freeze lock.
> 
> Also add two variants of memsave version.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

I hope you will spin another patch replacing '||' with '&&' in 
blk_queue_enter and __bio_queue_enter as you mentioned in another
mail. With that change, this looks good to me:

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

Re: [PATCH 3/3] block: use blk_mq_no_io() for avoiding lock dependency

[Ming Lei]

On Wed, Apr 02, 2025 at 07:13:56PM +0530, Nilay Shroff wrote:
> 
> 
> On 4/2/25 10:08 AM, Ming Lei wrote:
> > Use blk_mq_no_io() to prevent IO from entering queue for avoiding lock
> > dependency between freeze lock and elevator lock, and we have got many
> > such reports:
> > 
> > Reported-by: syzbot+4c7e0f9b94ad65811efb@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/linux-block/67e6b425.050a0220.2f068f.007b.GAE@google.com/
> > Reported-by: Valdis Klētnieks <valdis.kletnieks@vt.edu>
> > Closes: https://lore.kernel.org/linux-block/7755.1743228130@turing-police/#t
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> 
> I tested this series on my system and this works well as we cut dependency
> between ->elevator_lock and ->freeze_lock. However don't we plan to now 
> model blk_mq_enter_no_io and blk_mq_exit_no_io as lock/unlock for supporting 
> lockdep? Maybe we don't.

Good point!

> 
> Overall changes looks good to me:
> Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

Thanks for the review!

lockdep modeling for blk_mq_enter_no_io and blk_mq_exit_no_io has been
added in V2.


Thanks, 
Ming