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

[Nilay Shroff <nilay@linux.ibm.com>]

On 10/24/26 1:47 AM, Laveesh Bansal wrote:
> 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