]> www.infradead.org Git - users/jedix/linux-maple.git/commitdiff
block: fix lockdep warning caused by lock dependency in elv_iosched_store
authorNilay Shroff <nilay@linux.ibm.com>
Wed, 30 Jul 2025 07:46:08 +0000 (13:16 +0530)
committerJens Axboe <axboe@kernel.dk>
Wed, 30 Jul 2025 12:20:51 +0000 (06:20 -0600)
Recent lockdep reports [1] have revealed a potential deadlock caused by a
lock dependency between the percpu allocator lock and the elevator lock.
This issue can be avoided by ensuring that the allocation and release of
scheduler tags (sched_tags) are performed outside the elevator lock.
Furthermore, the queue does not need to be remain frozen during these
operations.

To address this, move all sched_tags allocations and deallocations outside
of both the ->elevator_lock and the ->freeze_lock. Since the lifetime of
the elevator queue and its associated sched_tags is closely tied, the
allocated sched_tags are now stored in the elevator queue structure. Then,
during the actual elevator switch (which runs under ->freeze_lock and
->elevator_lock), the pre-allocated sched_tags are assigned to the
appropriate q->hctx. Once the elevator switch is complete and the locks
are released, the old elevator queue and its associated sched_tags are
freed.

This commit specifically addresses the allocation/deallocation of sched_
tags during elevator switching. Note that sched_tags may also be allocated
in other contexts, such as during nr_hw_queues updates. Supporting that
use case will require batch allocation/deallocation, which will be handled
in a follow-up patch.

This restructuring ensures that sched_tags memory management occurs
entirely outside of the ->elevator_lock and ->freeze_lock context,
eliminating the lock dependency problem seen during scheduler updates.

[1] https://lore.kernel.org/all/0659ea8d-a463-47c8-9180-43c719e106eb@linux.ibm.com/

Reported-by: Stefan Haberland <sth@linux.ibm.com>
Closes: https://lore.kernel.org/all/0659ea8d-a463-47c8-9180-43c719e106eb@linux.ibm.com/
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
Link: https://lore.kernel.org/r/20250730074614.2537382-3-nilay@linux.ibm.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
block/blk-mq-sched.c
block/blk-mq-sched.h
block/elevator.c
block/elevator.h

index 359e0704e09be5a002291dad51f459750efee3a0..2d6d1ebdd8fbe01d9eadad0865edd873cac34b32 100644 (file)
@@ -374,64 +374,17 @@ bool blk_mq_sched_try_insert_merge(struct request_queue *q, struct request *rq,
 }
 EXPORT_SYMBOL_GPL(blk_mq_sched_try_insert_merge);
 
-static int blk_mq_sched_alloc_map_and_rqs(struct request_queue *q,
-                                         struct blk_mq_hw_ctx *hctx,
-                                         unsigned int hctx_idx)
-{
-       if (blk_mq_is_shared_tags(q->tag_set->flags)) {
-               hctx->sched_tags = q->sched_shared_tags;
-               return 0;
-       }
-
-       hctx->sched_tags = blk_mq_alloc_map_and_rqs(q->tag_set, hctx_idx,
-                                                   q->nr_requests);
-
-       if (!hctx->sched_tags)
-               return -ENOMEM;
-       return 0;
-}
-
-static void blk_mq_exit_sched_shared_tags(struct request_queue *queue)
-{
-       blk_mq_free_rq_map(queue->sched_shared_tags);
-       queue->sched_shared_tags = NULL;
-}
-
 /* called in queue's release handler, tagset has gone away */
 static void blk_mq_sched_tags_teardown(struct request_queue *q, unsigned int flags)
 {
        struct blk_mq_hw_ctx *hctx;
        unsigned long i;
 
-       queue_for_each_hw_ctx(q, hctx, i) {
-               if (hctx->sched_tags) {
-                       if (!blk_mq_is_shared_tags(flags))
-                               blk_mq_free_rq_map(hctx->sched_tags);
-                       hctx->sched_tags = NULL;
-               }
-       }
+       queue_for_each_hw_ctx(q, hctx, i)
+               hctx->sched_tags = NULL;
 
        if (blk_mq_is_shared_tags(flags))
-               blk_mq_exit_sched_shared_tags(q);
-}
-
-static int blk_mq_init_sched_shared_tags(struct request_queue *queue)
-{
-       struct blk_mq_tag_set *set = queue->tag_set;
-
-       /*
-        * Set initial depth at max so that we don't need to reallocate for
-        * updating nr_requests.
-        */
-       queue->sched_shared_tags = blk_mq_alloc_map_and_rqs(set,
-                                               BLK_MQ_NO_HCTX_IDX,
-                                               MAX_SCHED_RQ);
-       if (!queue->sched_shared_tags)
-               return -ENOMEM;
-
-       blk_mq_tag_update_sched_shared_tags(queue);
-
-       return 0;
+               q->sched_shared_tags = NULL;
 }
 
 void blk_mq_sched_reg_debugfs(struct request_queue *q)
@@ -458,8 +411,75 @@ void blk_mq_sched_unreg_debugfs(struct request_queue *q)
        mutex_unlock(&q->debugfs_mutex);
 }
 
+void blk_mq_free_sched_tags(struct elevator_tags *et,
+               struct blk_mq_tag_set *set)
+{
+       unsigned long i;
+
+       /* Shared tags are stored at index 0 in @tags. */
+       if (blk_mq_is_shared_tags(set->flags))
+               blk_mq_free_map_and_rqs(set, et->tags[0], BLK_MQ_NO_HCTX_IDX);
+       else {
+               for (i = 0; i < et->nr_hw_queues; i++)
+                       blk_mq_free_map_and_rqs(set, et->tags[i], i);
+       }
+
+       kfree(et);
+}
+
+struct elevator_tags *blk_mq_alloc_sched_tags(struct blk_mq_tag_set *set,
+               unsigned int nr_hw_queues)
+{
+       unsigned int nr_tags;
+       int i;
+       struct elevator_tags *et;
+       gfp_t gfp = GFP_NOIO | __GFP_ZERO | __GFP_NOWARN | __GFP_NORETRY;
+
+       if (blk_mq_is_shared_tags(set->flags))
+               nr_tags = 1;
+       else
+               nr_tags = nr_hw_queues;
+
+       et = kmalloc(sizeof(struct elevator_tags) +
+                       nr_tags * sizeof(struct blk_mq_tags *), gfp);
+       if (!et)
+               return NULL;
+       /*
+        * Default to double of smaller one between hw queue_depth and
+        * 128, since we don't split into sync/async like the old code
+        * did. Additionally, this is a per-hw queue depth.
+        */
+       et->nr_requests = 2 * min_t(unsigned int, set->queue_depth,
+                       BLKDEV_DEFAULT_RQ);
+       et->nr_hw_queues = nr_hw_queues;
+
+       if (blk_mq_is_shared_tags(set->flags)) {
+               /* Shared tags are stored at index 0 in @tags. */
+               et->tags[0] = blk_mq_alloc_map_and_rqs(set, BLK_MQ_NO_HCTX_IDX,
+                                       MAX_SCHED_RQ);
+               if (!et->tags[0])
+                       goto out;
+       } else {
+               for (i = 0; i < et->nr_hw_queues; i++) {
+                       et->tags[i] = blk_mq_alloc_map_and_rqs(set, i,
+                                       et->nr_requests);
+                       if (!et->tags[i])
+                               goto out_unwind;
+               }
+       }
+
+       return et;
+out_unwind:
+       while (--i >= 0)
+               blk_mq_free_map_and_rqs(set, et->tags[i], i);
+out:
+       kfree(et);
+       return NULL;
+}
+
 /* caller must have a reference to @e, will grab another one if successful */
-int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
+int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e,
+               struct elevator_tags *et)
 {
        unsigned int flags = q->tag_set->flags;
        struct blk_mq_hw_ctx *hctx;
@@ -467,40 +487,33 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
        unsigned long i;
        int ret;
 
-       /*
-        * Default to double of smaller one between hw queue_depth and 128,
-        * since we don't split into sync/async like the old code did.
-        * Additionally, this is a per-hw queue depth.
-        */
-       q->nr_requests = 2 * min_t(unsigned int, q->tag_set->queue_depth,
-                                  BLKDEV_DEFAULT_RQ);
-
-       eq = elevator_alloc(q, e);
+       eq = elevator_alloc(q, e, et);
        if (!eq)
                return -ENOMEM;
 
+       q->nr_requests = et->nr_requests;
+
        if (blk_mq_is_shared_tags(flags)) {
-               ret = blk_mq_init_sched_shared_tags(q);
-               if (ret)
-                       goto err_put_elevator;
+               /* Shared tags are stored at index 0 in @et->tags. */
+               q->sched_shared_tags = et->tags[0];
+               blk_mq_tag_update_sched_shared_tags(q);
        }
 
        queue_for_each_hw_ctx(q, hctx, i) {
-               ret = blk_mq_sched_alloc_map_and_rqs(q, hctx, i);
-               if (ret)
-                       goto err_free_map_and_rqs;
+               if (blk_mq_is_shared_tags(flags))
+                       hctx->sched_tags = q->sched_shared_tags;
+               else
+                       hctx->sched_tags = et->tags[i];
        }
 
        ret = e->ops.init_sched(q, eq);
        if (ret)
-               goto err_free_map_and_rqs;
+               goto out;
 
        queue_for_each_hw_ctx(q, hctx, i) {
                if (e->ops.init_hctx) {
                        ret = e->ops.init_hctx(hctx, i);
                        if (ret) {
-                               eq = q->elevator;
-                               blk_mq_sched_free_rqs(q);
                                blk_mq_exit_sched(q, eq);
                                kobject_put(&eq->kobj);
                                return ret;
@@ -509,10 +522,8 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
        }
        return 0;
 
-err_free_map_and_rqs:
-       blk_mq_sched_free_rqs(q);
+out:
        blk_mq_sched_tags_teardown(q, flags);
-err_put_elevator:
        kobject_put(&eq->kobj);
        q->elevator = NULL;
        return ret;
index 1326526bb7338ce28989101b896d5d2d58864f5f..0cde00cd1c4714a8ca7725691672e4a2283920c3 100644 (file)
@@ -18,10 +18,16 @@ void __blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx);
 
 void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx);
 
-int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e);
+int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e,
+               struct elevator_tags *et);
 void blk_mq_exit_sched(struct request_queue *q, struct elevator_queue *e);
 void blk_mq_sched_free_rqs(struct request_queue *q);
 
+struct elevator_tags *blk_mq_alloc_sched_tags(struct blk_mq_tag_set *set,
+               unsigned int nr_hw_queues);
+void blk_mq_free_sched_tags(struct elevator_tags *et,
+               struct blk_mq_tag_set *set);
+
 static inline void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
 {
        if (test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state))
index 939b0c590fbe954ad62230b17122e7011a19f25d..e9dc837b7b7040984b06c65ba17751cd6958091f 100644 (file)
@@ -54,6 +54,8 @@ struct elv_change_ctx {
        struct elevator_queue *old;
        /* for registering new elevator */
        struct elevator_queue *new;
+       /* holds sched tags data */
+       struct elevator_tags *et;
 };
 
 static DEFINE_SPINLOCK(elv_list_lock);
@@ -132,7 +134,7 @@ static struct elevator_type *elevator_find_get(const char *name)
 static const struct kobj_type elv_ktype;
 
 struct elevator_queue *elevator_alloc(struct request_queue *q,
-                                 struct elevator_type *e)
+               struct elevator_type *e, struct elevator_tags *et)
 {
        struct elevator_queue *eq;
 
@@ -145,6 +147,7 @@ struct elevator_queue *elevator_alloc(struct request_queue *q,
        kobject_init(&eq->kobj, &elv_ktype);
        mutex_init(&eq->sysfs_lock);
        hash_init(eq->hash);
+       eq->et = et;
 
        return eq;
 }
@@ -165,7 +168,6 @@ static void elevator_exit(struct request_queue *q)
        lockdep_assert_held(&q->elevator_lock);
 
        ioc_clear_queue(q);
-       blk_mq_sched_free_rqs(q);
 
        mutex_lock(&e->sysfs_lock);
        blk_mq_exit_sched(q, e);
@@ -591,7 +593,7 @@ static int elevator_switch(struct request_queue *q, struct elv_change_ctx *ctx)
        }
 
        if (new_e) {
-               ret = blk_mq_init_sched(q, new_e);
+               ret = blk_mq_init_sched(q, new_e, ctx->et);
                if (ret)
                        goto out_unfreeze;
                ctx->new = q->elevator;
@@ -626,8 +628,10 @@ static void elv_exit_and_release(struct request_queue *q)
        elevator_exit(q);
        mutex_unlock(&q->elevator_lock);
        blk_mq_unfreeze_queue(q, memflags);
-       if (e)
+       if (e) {
+               blk_mq_free_sched_tags(e->et, q->tag_set);
                kobject_put(&e->kobj);
+       }
 }
 
 static int elevator_change_done(struct request_queue *q,
@@ -640,6 +644,7 @@ static int elevator_change_done(struct request_queue *q,
                                &ctx->old->flags);
 
                elv_unregister_queue(q, ctx->old);
+               blk_mq_free_sched_tags(ctx->old->et, q->tag_set);
                kobject_put(&ctx->old->kobj);
                if (enable_wbt)
                        wbt_enable_default(q->disk);
@@ -658,9 +663,16 @@ static int elevator_change_done(struct request_queue *q,
 static int elevator_change(struct request_queue *q, struct elv_change_ctx *ctx)
 {
        unsigned int memflags;
+       struct blk_mq_tag_set *set = q->tag_set;
        int ret = 0;
 
-       lockdep_assert_held(&q->tag_set->update_nr_hwq_lock);
+       lockdep_assert_held(&set->update_nr_hwq_lock);
+
+       if (strncmp(ctx->name, "none", 4)) {
+               ctx->et = blk_mq_alloc_sched_tags(set, set->nr_hw_queues);
+               if (!ctx->et)
+                       return -ENOMEM;
+       }
 
        memflags = blk_mq_freeze_queue(q);
        /*
@@ -680,6 +692,11 @@ static int elevator_change(struct request_queue *q, struct elv_change_ctx *ctx)
        blk_mq_unfreeze_queue(q, memflags);
        if (!ret)
                ret = elevator_change_done(q, ctx);
+       /*
+        * Free sched tags if it's allocated but we couldn't switch elevator.
+        */
+       if (ctx->et && !ctx->new)
+               blk_mq_free_sched_tags(ctx->et, set);
 
        return ret;
 }
@@ -690,6 +707,7 @@ static int elevator_change(struct request_queue *q, struct elv_change_ctx *ctx)
  */
 void elv_update_nr_hw_queues(struct request_queue *q, struct elevator_type *e)
 {
+       struct blk_mq_tag_set *set = q->tag_set;
        struct elv_change_ctx ctx = {};
        int ret = -ENODEV;
 
@@ -697,15 +715,25 @@ void elv_update_nr_hw_queues(struct request_queue *q, struct elevator_type *e)
 
        if (e && !blk_queue_dying(q) && blk_queue_registered(q)) {
                ctx.name = e->elevator_name;
-
+               ctx.et = blk_mq_alloc_sched_tags(set, set->nr_hw_queues);
+               if (!ctx.et) {
+                       WARN_ON_ONCE(1);
+                       goto unfreeze;
+               }
                mutex_lock(&q->elevator_lock);
                /* force to reattach elevator after nr_hw_queue is updated */
                ret = elevator_switch(q, &ctx);
                mutex_unlock(&q->elevator_lock);
        }
+unfreeze:
        blk_mq_unfreeze_queue_nomemrestore(q);
        if (!ret)
                WARN_ON_ONCE(elevator_change_done(q, &ctx));
+       /*
+        * Free sched tags if it's allocated but we couldn't switch elevator.
+        */
+       if (ctx.et && !ctx.new)
+               blk_mq_free_sched_tags(ctx.et, set);
 }
 
 /*
index a4de5f9ad7907700de4474176c20b73976f25844..adc5c157e17e51de90832a412af13e58816e8c8a 100644 (file)
@@ -23,6 +23,15 @@ enum elv_merge {
 struct blk_mq_alloc_data;
 struct blk_mq_hw_ctx;
 
+struct elevator_tags {
+       /* num. of hardware queues for which tags are allocated */
+       unsigned int nr_hw_queues;
+       /* depth used while allocating tags */
+       unsigned int nr_requests;
+       /* shared tag is stored at index 0 */
+       struct blk_mq_tags *tags[];
+};
+
 struct elevator_mq_ops {
        int (*init_sched)(struct request_queue *, struct elevator_queue *);
        void (*exit_sched)(struct elevator_queue *);
@@ -113,6 +122,7 @@ struct request *elv_rqhash_find(struct request_queue *q, sector_t offset);
 struct elevator_queue
 {
        struct elevator_type *type;
+       struct elevator_tags *et;
        void *elevator_data;
        struct kobject kobj;
        struct mutex sysfs_lock;
@@ -152,8 +162,8 @@ ssize_t elv_iosched_show(struct gendisk *disk, char *page);
 ssize_t elv_iosched_store(struct gendisk *disk, const char *page, size_t count);
 
 extern bool elv_bio_merge_ok(struct request *, struct bio *);
-extern struct elevator_queue *elevator_alloc(struct request_queue *,
-                                       struct elevator_type *);
+struct elevator_queue *elevator_alloc(struct request_queue *,
+               struct elevator_type *, struct elevator_tags *);
 
 /*
  * Helper functions.