Re: [PATCH] block: only serialize elevator switches of the same queue
flat view
From: Nilay Shroff <hidden>
Date: 2026-10-28 12:22:05
Also in:
lkml
Subsystem:
block layer, the rest · Maintainers:
Jens Axboe, Linus Torvalds
On 10/24/26 1:47 AM, Laveesh Bansal wrote:
quoted hunk ↗ jump to hunk
diff --git a/block/blk-core.c b/block/blk-core.c index 196bccf27f58..655c18237967 100644 --- a/block/blk-core.c +++ b/block/blk-core.c@@ -473,6 +473,7 @@ struct request_queue *blk_alloc_queue(struct queue_limits *lim, int node_id) refcount_set(&q->refs, 1); mutex_init(&q->debugfs_mutex); mutex_init(&q->elevator_lock); + mutex_init(&q->elevator_store_lock); mutex_init(&q->sysfs_lock); mutex_init(&q->limits_lock); mutex_init(&q->rq_qos_mutex);diff --git a/block/elevator.c b/block/elevator.c index 2161b6eea680..0a8fcb4bad06 100644 --- a/block/elevator.c +++ b/block/elevator.c@@ -812,16 +812,16 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf, * reference during concurrent disk deletion: * update_nr_hwq_lock -> kn->active (via del_gendisk -> kobject_del) * kn->active -> update_nr_hwq_lock (via this sysfs write path) - * - * Use the writer lock instead of the reader lock of update_nr_hwq_lock - * to serialize the two-stage elevator switch steps in - * elevator_change(): the core switch step under the elevator lock and - * the elevator_change_done() step outside the elevator lock. */ - if (!down_write_trylock(&set->update_nr_hwq_lock)) { + if (!down_read_trylock(&set->update_nr_hwq_lock)) { ret = -EBUSY; goto out; } + + /* The reader lock is shared: serialize both stages of a switch here. */ + ret = mutex_lock_killable(&q->elevator_store_lock);
We generally try to avoid introducing new locks unless absolutely necessary. Adding another lock to the generic block layer also introduces maintenance overhead. In fact, we have been trying to eliminate some existing locks in the block layer by restructuring the code where possible. That said, I understand the issue you're trying to address. Instead of introducing a new lock, I'd suggest using QUEUE_FLAG_NO_ELV_SWITCH in the sysfs store path. The store path could atomically test and set this flag. If the flag is already set, it means either another concurrent store operation is in progress or the disk is being torn down. In either case, the store operation can return -EBUSY. Based on this approach, I'd suggest the following change:
diff --git a/block/elevator.c b/block/elevator.c
index 2161b6eea680..e59381922679 100644
--- a/block/elevator.c
+++ b/block/elevator.c@@ -812,25 +812,24 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf, * reference during concurrent disk deletion: * update_nr_hwq_lock -> kn->active (via del_gendisk -> kobject_del) * kn->active -> update_nr_hwq_lock (via this sysfs write path) - * - * Use the writer lock instead of the reader lock of update_nr_hwq_lock - * to serialize the two-stage elevator switch steps in - * elevator_change(): the core switch step under the elevator lock and - * the elevator_change_done() step outside the elevator lock. */ - if (!down_write_trylock(&set->update_nr_hwq_lock)) { + if (!down_read_trylock(&set->update_nr_hwq_lock)) { ret = -EBUSY; goto out; } - if (!blk_queue_no_elv_switch(q)) { - ret = elevator_change(q, &ctx); - if (!ret) - ret = count; - } else { - ret = -ENOENT; + + if (test_and_set_bit(QUEUE_FLAG_NO_ELV_SWITCH, &q->queue_flags)) { + ret = -EBUSY; + goto out_unlock; } - up_write(&set->update_nr_hwq_lock); + ret = elevator_change(q, &ctx); + if (!ret) + ret = count; + + clear_bit(QUEUE_FLAG_NO_ELV_SWITCH, &q->queue_flags); +out_unlock: + up_read(&set->update_nr_hwq_lock); out: if (ctx.type) elevator_put(ctx.type);
Please note that I have only compile tested the above code. You may test it,
and update if this works for your case.
Thanks,
--Nilay