block: model freeze & enter queue as lock for supporting lockdep

Recently we got several deadlock report[1][2][3] caused by
blk_mq_freeze_queue and blk_enter_queue().

Turns out the two are just like acquiring read/write lock, so model them
as read/write lock for supporting lockdep:

1) model q->q_usage_counter as two locks(io and queue lock)

- queue lock covers sync with blk_enter_queue()

- io lock covers sync with bio_enter_queue()

2) make the lockdep class/key as per-queue:

- different subsystem has very different lock use pattern, shared lock
 class causes false positive easily

- freeze_queue degrades to no lock in case that disk state becomes DEAD
  because bio_enter_queue() won't be blocked any more

- freeze_queue degrades to no lock in case that request queue becomes dying
  because blk_enter_queue() won't be blocked any more

3) model blk_mq_freeze_queue() as acquire_exclusive & try_lock
- it is exclusive lock, so dependency with blk_enter_queue() is covered

- it is trylock because blk_mq_freeze_queue() are allowed to run
  concurrently

4) model blk_enter_queue() & bio_enter_queue() as acquire_read()
- nested blk_enter_queue() are allowed

- dependency with blk_mq_freeze_queue() is covered

- blk_queue_exit() is often called from other contexts(such as irq), and
it can't be annotated as lock_release(), so simply do it in
blk_enter_queue(), this way still covered cases as many as possible

With lockdep support, such kind of reports may be reported asap and
needn't wait until the real deadlock is triggered.

For example, lockdep report can be triggered in the report[3] with this
patch applied.

[1] occasional block layer hang when setting 'echo noop > /sys/block/sda/queue/scheduler'
https://bugzilla.kernel.org/show_bug.cgi?id=219166

[2] del_gendisk() vs blk_queue_enter() race condition
https://lore.kernel.org/linux-block/20241003085610.GK11458@google.com/

[3] queue_freeze & queue_enter deadlock in scsi
https://lore.kernel.org/linux-block/ZxG38G9BuFdBpBHZ@fedora/T/#u

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
Link: https://lore.kernel.org/r/20241025003722.3630252-4-ming.lei@redhat.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/block/blk-core.c b/block/blk-core.c
index bc5e8c5eaac9ffc2ebcab5ea0f08fd9a28e6facb..09d10bb95fda0526588c42b4421b54a5ceac1369 100644 (file)
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -261,6 +261,8 @@ static void blk_free_queue(struct request_queue *q)
                blk_mq_release(q);
 
        ida_free(&blk_queue_ida, q->id);
+       lockdep_unregister_key(&q->io_lock_cls_key);
+       lockdep_unregister_key(&q->q_lock_cls_key);
        call_rcu(&q->rcu_head, blk_free_queue_rcu);
 }
 
@@ -278,18 +280,20 @@ void blk_put_queue(struct request_queue *q)
 }
 EXPORT_SYMBOL(blk_put_queue);
 
-void blk_queue_start_drain(struct request_queue *q)
+bool blk_queue_start_drain(struct request_queue *q)
 {
        /*
         * When queue DYING flag is set, we need to block new req
         * entering queue, so we call blk_freeze_queue_start() to
         * prevent I/O from crossing blk_queue_enter().
         */
-       blk_freeze_queue_start(q);
+       bool freeze = __blk_freeze_queue_start(q);
        if (queue_is_mq(q))
                blk_mq_wake_waiters(q);
        /* Make blk_queue_enter() reexamine the DYING flag. */
        wake_up_all(&q->mq_freeze_wq);
+
+       return freeze;
 }
 
 /**
@@ -321,6 +325,8 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
                        return -ENODEV;
        }
 
+       rwsem_acquire_read(&q->q_lockdep_map, 0, 0, _RET_IP_);
+       rwsem_release(&q->q_lockdep_map, _RET_IP_);
        return 0;
 }
 
@@ -352,6 +358,8 @@ int __bio_queue_enter(struct request_queue *q, struct bio *bio)
                        goto dead;
        }
 
+       rwsem_acquire_read(&q->io_lockdep_map, 0, 0, _RET_IP_);
+       rwsem_release(&q->io_lockdep_map, _RET_IP_);
        return 0;
 dead:
        bio_io_error(bio);
@@ -441,6 +449,12 @@ struct request_queue *blk_alloc_queue(struct queue_limits *lim, int node_id)
                                PERCPU_REF_INIT_ATOMIC, GFP_KERNEL);
        if (error)
                goto fail_stats;
+       lockdep_register_key(&q->io_lock_cls_key);
+       lockdep_register_key(&q->q_lock_cls_key);
+       lockdep_init_map(&q->io_lockdep_map, "&q->q_usage_counter(io)",
+                        &q->io_lock_cls_key, 0);
+       lockdep_init_map(&q->q_lockdep_map, "&q->q_usage_counter(queue)",
+                        &q->q_lock_cls_key, 0);
 
        q->nr_requests = BLKDEV_DEFAULT_RQ;
 

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 77027681550700650348bec3499ef7c93a008f0f..4ae7eb335fbdec5545308da131ba1c75374eacdc 100644 (file)
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -120,17 +120,29 @@ 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)
+bool __blk_freeze_queue_start(struct request_queue *q)
 {
+       int freeze;
+
        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);
+               freeze = true;
        } else {
                mutex_unlock(&q->mq_freeze_lock);
+               freeze = false;
        }
+
+       return freeze;
+}
+
+void blk_freeze_queue_start(struct request_queue *q)
+{
+       if (__blk_freeze_queue_start(q))
+               blk_freeze_acquire_lock(q, false, false);
 }
 EXPORT_SYMBOL_GPL(blk_freeze_queue_start);
 
@@ -176,8 +188,10 @@ void blk_mq_freeze_queue(struct request_queue *q)
 }
 EXPORT_SYMBOL_GPL(blk_mq_freeze_queue);
 
-void __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic)
+bool __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic)
 {
+       int unfreeze = false;
+
        mutex_lock(&q->mq_freeze_lock);
        if (force_atomic)
                q->q_usage_counter.data->force_atomic = true;
@@ -186,13 +200,17 @@ void __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;
        }
        mutex_unlock(&q->mq_freeze_lock);
+
+       return unfreeze;
 }
 
 void blk_mq_unfreeze_queue(struct request_queue *q)
 {
-       __blk_mq_unfreeze_queue(q, false);
+       if (__blk_mq_unfreeze_queue(q, false))
+               blk_unfreeze_release_lock(q, false, false);
 }
 EXPORT_SYMBOL_GPL(blk_mq_unfreeze_queue);
 
@@ -205,7 +223,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);
 }
 EXPORT_SYMBOL_GPL(blk_freeze_queue_start_non_owner);
 

diff --git a/block/blk.h b/block/blk.h
index 8fddaf6eae49da23921ad0851a4ade2341b9ff31..63d5df0dc29c0578ae3f1efef0dcf706ebdcc0be 100644 (file)
--- a/block/blk.h
+++ b/block/blk.h
@@ -4,6 +4,7 @@
 
 #include <linux/bio-integrity.h>
 #include <linux/blk-crypto.h>
+#include <linux/lockdep.h>
 #include <linux/memblock.h>    /* for max_pfn/max_low_pfn */
 #include <linux/sched/sysctl.h>
 #include <linux/timekeeping.h>
@@ -35,8 +36,9 @@ struct blk_flush_queue *blk_alloc_flush_queue(int node, int cmd_size,
 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);
+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);
 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);
@@ -69,8 +71,11 @@ static inline int bio_queue_enter(struct bio *bio)
 {
        struct request_queue *q = bdev_get_queue(bio->bi_bdev);
 
-       if (blk_try_enter_queue(q, false))
+       if (blk_try_enter_queue(q, false)) {
+               rwsem_acquire_read(&q->io_lockdep_map, 0, 0, _RET_IP_);
+               rwsem_release(&q->io_lockdep_map, _RET_IP_);
                return 0;
+       }
        return __bio_queue_enter(q, bio);
 }
 
@@ -724,4 +729,22 @@ void blk_integrity_verify(struct bio *bio);
 void blk_integrity_prepare(struct request *rq);
 void blk_integrity_complete(struct request *rq, unsigned int nr_bytes);
 
+static inline void blk_freeze_acquire_lock(struct request_queue *q, bool
+               disk_dead, bool queue_dying)
+{
+       if (!disk_dead)
+               rwsem_acquire(&q->io_lockdep_map, 0, 1, _RET_IP_);
+       if (!queue_dying)
+               rwsem_acquire(&q->q_lockdep_map, 0, 1, _RET_IP_);
+}
+
+static inline void blk_unfreeze_release_lock(struct request_queue *q, bool
+               disk_dead, bool queue_dying)
+{
+       if (!queue_dying)
+               rwsem_release(&q->q_lockdep_map, _RET_IP_);
+       if (!disk_dead)
+               rwsem_release(&q->io_lockdep_map, _RET_IP_);
+}
+
 #endif /* BLK_INTERNAL_H */

diff --git a/block/genhd.c b/block/genhd.c
index bc30eee7ab16f01b5bbc50e53503bff75790d4e1..dfee66146bd1ff8f0c833b8fd56f8dcbbe045e5d 100644 (file)
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -601,13 +601,13 @@ static void blk_report_disk_dead(struct gendisk *disk, bool surprise)
        rcu_read_unlock();
 }
 
-static void __blk_mark_disk_dead(struct gendisk *disk)
+static bool __blk_mark_disk_dead(struct gendisk *disk)
 {
        /*
         * Fail any new I/O.
         */
        if (test_and_set_bit(GD_DEAD, &disk->state))
-               return;
+               return false;
 
        if (test_bit(GD_OWNS_QUEUE, &disk->state))
                blk_queue_flag_set(QUEUE_FLAG_DYING, disk->queue);
@@ -620,7 +620,7 @@ static void __blk_mark_disk_dead(struct gendisk *disk)
        /*
         * Prevent new I/O from crossing bio_queue_enter().
         */
-       blk_queue_start_drain(disk->queue);
+       return blk_queue_start_drain(disk->queue);
 }
 
 /**
@@ -661,6 +661,7 @@ void del_gendisk(struct gendisk *disk)
        struct request_queue *q = disk->queue;
        struct block_device *part;
        unsigned long idx;
+       bool start_drain, queue_dying;
 
        might_sleep();
 
@@ -688,7 +689,10 @@ void del_gendisk(struct gendisk *disk)
         * Drop all partitions now that the disk is marked dead.
         */
        mutex_lock(&disk->open_mutex);
-       __blk_mark_disk_dead(disk);
+       start_drain = __blk_mark_disk_dead(disk);
+       queue_dying = blk_queue_dying(q);
+       if (start_drain)
+               blk_freeze_acquire_lock(q, true, queue_dying);
        xa_for_each_start(&disk->part_tbl, idx, part, 1)
                drop_partition(part);
        mutex_unlock(&disk->open_mutex);
@@ -745,6 +749,9 @@ void del_gendisk(struct gendisk *disk)
                if (queue_is_mq(q))
                        blk_mq_exit_queue(q);
        }
+
+       if (start_drain)
+               blk_unfreeze_release_lock(q, true, queue_dying);
 }
 EXPORT_SYMBOL(del_gendisk);
 

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 55bec14fe55f947feeb4f56fe8a56413781f0eee..d0a52ed05e60c480ecc02b95649bf1f3348c103c 100644 (file)
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -25,6 +25,7 @@
 #include <linux/uuid.h>
 #include <linux/xarray.h>
 #include <linux/file.h>
+#include <linux/lockdep.h>
 
 struct module;
 struct request_queue;
@@ -474,6 +475,11 @@ struct request_queue {
        struct xarray           hctx_table;
 
        struct percpu_ref       q_usage_counter;
+       struct lock_class_key   io_lock_cls_key;
+       struct lockdep_map      io_lockdep_map;
+
+       struct lock_class_key   q_lock_cls_key;
+       struct lockdep_map      q_lockdep_map;
 
        struct request          *last_merge;