[PATCH v2] block: only serialize elevator switches of the same queue

[PATCH v2] block: only serialize elevator switches of the same queue

[Laveesh Bansal]

Since commit 4ff58d6bc9dd ("block: serialize elevator changes for the
same queue using a writer lock"), a write to queue/scheduler fails with
-EBUSY whenever something else holds set->update_nr_hwq_lock. That
commit takes the lock for write, and it has to be a trylock because of
commit ce8ee8583ed8 ("block: use trylock to avoid lockdep circular
dependency in sysfs").

The lock covers the whole tag set, so it is easy to hit. A scheduler
write fails while another queue of the set is switching, or while a
sibling disk of the same SCSI host or NVMe controller is added or
removed. This matches the udev failures reported on 7.2.

Go back to taking the lock for read. The write lock was there to keep
two switches of one queue from overlapping in elevator_change_done(),
which touches the elevator sysfs and debugfs entries outside of
q->elevator_lock. Use QUEUE_FLAG_NO_ELV_SWITCH for that instead: a
write sets it before elevator_change() and clears it afterwards, and a
write that finds it set returns -EBUSY. Disk removal already sets the
flag under the write lock, so no switch can start once it has run.

Writes to the same queue, and writes that race with an nr_hw_queues
update, an nr_requests write or disk removal, can still get -EBUSY, as
with 4ff58d6bc9dd. A write to a disk that is being removed now gets
-EBUSY instead of -ENOENT.

Fixes: 4ff58d6bc9dd ("block: serialize elevator changes for the same queue using a writer lock")
Cc: stable@vger.kernel.org # v7.2+
Reported-by: Reindl Harald <h.reindl@thelounge.net>
Closes: https://lore.kernel.org/r/077dd42e-cbc9-4c23-a5ef-3d20e4fc7757@thelounge.net
Suggested-by: Nilay Shroff <nilay@linux.ibm.com>
Assisted-by: LLM
Signed-off-by: Laveesh Bansal <laveeshb@laveeshbansal.com>
---
Changes since v1 (https://lore.kernel.org/r/20261023201704.20577-1-laveeshb@laveeshbansal.com):
- Replaced the new mutex with QUEUE_FLAG_NO_ELV_SWITCH as a busy bit
  under the read lock, as Nilay suggested
  (https://lore.kernel.org/r/7f697f9d-6e8c-4544-bca7-137d3f3d2dbf@linux.ibm.com).
- Shin'ichiro's Tested-by on v1 does not carry over, since the code changed.
- I also tried taking q->sysfs_lock around the switch instead. It fixes
  the report too, but same-queue writers wait on it, and 1bf70d08cc3b
  ("block: introduce a dedicated lock for protecting queue elevator
  updates") moved elevator switching off that lock, so I kept the flag.

Tested on arm64 and x86_64 in QEMU, with KASAN, lockdep and debug lists.
Writes to queue/scheduler failing with -EBUSY, one number per run
(writers x writes):

                                       x86_64        arm64
  one writer per namespace (NVMe, one controller)
    v7.3, 4 x 400                      572, 649, 597 704, 662
    this patch, 4 x 400                0, 0          0, 0
  one writer, sibling namespaces added and removed
    v7.3, 1 x 400                      23, 32, 34    42, 43
    this patch, 1 x 400                0, 0, 1       0, 4, 1
  four writers on the same queue (null_blk)
    v7.3, 4 x 400                      849, 795, 756 807, 926
    this patch, 4 x 400                779, 790      910, 816
  one writer per SCSI disk (scsi_debug, 4 LUNs on one host)
    v7.3, 4 x 400                      828, 818      -
    this patch, 4 x 400                0, 0          -
  one writer on a SCSI disk, siblings deleted and rescanned
    v7.3, 1 x 400                      43, 46        -
    this patch, 1 x 400                0, 0          -

20 disk removals under writers complete on both architectures, with no
KASAN, lockdep or hung-task reports. blktests on x86_64 (all groups
except bcache, blktrace, rnbd and srp) gives the same results as v7.3.

 block/elevator.c       | 30 +++++++++++++++++-------------
 include/linux/blkdev.h |  4 +---
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index 2161b6eea680..66edf1a5768a 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -812,25 +812,29 @@ 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;
+
+	/*
+	 * The reader lock is shared, so use QUEUE_FLAG_NO_ELV_SWITCH to allow
+	 * only one switch of this queue at a time, and none once the disk is
+	 * going away.
+	 */
+	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;
+
+	blk_queue_flag_clear(QUEUE_FLAG_NO_ELV_SWITCH, q);
+out_unlock:
+	up_read(&set->update_nr_hwq_lock);
 out:
 	if (ctx.type)
 		elevator_put(ctx.type);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 4f7905c3412b..3989bb0dd43b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -675,7 +675,7 @@ enum {
 	QUEUE_FLAG_HCTX_ACTIVE,		/* at least one blk-mq hctx is active */
 	QUEUE_FLAG_SQ_SCHED,		/* single queue style io dispatch */
 	QUEUE_FLAG_DISABLE_WBT_DEF,	/* for sched to disable/enable wbt */
-	QUEUE_FLAG_NO_ELV_SWITCH,	/* can't switch elevator any more */
+	QUEUE_FLAG_NO_ELV_SWITCH,	/* a switch is running or disk is going away */
 	QUEUE_FLAG_QOS_ENABLED,		/* qos is enabled */
 	QUEUE_FLAG_BIO_ISSUE_TIME,	/* record bio->issue_time_ns */
 	QUEUE_FLAG_ZONED_QD1_WRITES,	/* Limit zoned devices writes to QD=1 */
@@ -716,8 +716,6 @@ void blk_queue_flag_clear(unsigned int flag, struct request_queue *q);
 	((q)->limits.features & BLK_FEAT_SKIP_TAGSET_QUIESCE)
 #define blk_queue_disable_wbt(q)	\
 	test_bit(QUEUE_FLAG_DISABLE_WBT_DEF, &(q)->queue_flags)
-#define blk_queue_no_elv_switch(q)	\
-	test_bit(QUEUE_FLAG_NO_ELV_SWITCH, &(q)->queue_flags)
 #define blk_queue_zoned_qd1_writes(q)	\
 	test_bit(QUEUE_FLAG_ZONED_QD1_WRITES, &(q)->queue_flags)
 
-- 
2.43.0