Re: [PATCH] block: protect debugfs attributes using q->elevator_lock

[Damien Le Moal <dlemoal@xxxxxxxxxx>]

On 3/12/25 19:28, Nilay Shroff wrote:
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index adf5f0697b6b..d26a6df945bd 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -347,11 +347,16 @@ static int hctx_busy_show(void *data, struct seq_file *m)
>  {
>  	struct blk_mq_hw_ctx *hctx = data;
>  	struct show_busy_params params = { .m = m, .hctx = hctx };
> +	int res;
>  
> +	res = mutex_lock_interruptible(&hctx->queue->elevator_lock);
> +	if (res)
> +		goto out;

There is no need for the goto here. You can "return res;" directly.
Same comment for all the other changes below.

>  	blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy_rq,
>  				&params);
> -
> -	return 0;
> +	mutex_unlock(&hctx->queue->elevator_lock);
> +out:
> +	return res;

And you can keep the "return 0;" here.

>  }
>  
>  static const char *const hctx_types[] = {
> @@ -400,12 +405,12 @@ static int hctx_tags_show(void *data, struct seq_file *m)
>  	struct request_queue *q = hctx->queue;
>  	int res;
>  
> -	res = mutex_lock_interruptible(&q->sysfs_lock);
> +	res = mutex_lock_interruptible(&q->elevator_lock);
>  	if (res)
>  		goto out;
>  	if (hctx->tags)
>  		blk_mq_debugfs_tags_show(m, hctx->tags);
> -	mutex_unlock(&q->sysfs_lock);
> +	mutex_unlock(&q->elevator_lock);
>  
>  out:
>  	return res;
> @@ -417,12 +422,12 @@ static int hctx_tags_bitmap_show(void *data, struct seq_file *m)
>  	struct request_queue *q = hctx->queue;
>  	int res;
>  
> -	res = mutex_lock_interruptible(&q->sysfs_lock);
> +	res = mutex_lock_interruptible(&q->elevator_lock);
>  	if (res)
>  		goto out;
>  	if (hctx->tags)
>  		sbitmap_bitmap_show(&hctx->tags->bitmap_tags.sb, m);
> -	mutex_unlock(&q->sysfs_lock);
> +	mutex_unlock(&q->elevator_lock);
>  
>  out:
>  	return res;
> @@ -434,12 +439,12 @@ static int hctx_sched_tags_show(void *data, struct seq_file *m)
>  	struct request_queue *q = hctx->queue;
>  	int res;
>  
> -	res = mutex_lock_interruptible(&q->sysfs_lock);
> +	res = mutex_lock_interruptible(&q->elevator_lock);
>  	if (res)
>  		goto out;
>  	if (hctx->sched_tags)
>  		blk_mq_debugfs_tags_show(m, hctx->sched_tags);
> -	mutex_unlock(&q->sysfs_lock);
> +	mutex_unlock(&q->elevator_lock);
>  
>  out:
>  	return res;
> @@ -451,12 +456,12 @@ static int hctx_sched_tags_bitmap_show(void *data, struct seq_file *m)
>  	struct request_queue *q = hctx->queue;
>  	int res;
>  
> -	res = mutex_lock_interruptible(&q->sysfs_lock);
> +	res = mutex_lock_interruptible(&q->elevator_lock);
>  	if (res)
>  		goto out;
>  	if (hctx->sched_tags)
>  		sbitmap_bitmap_show(&hctx->sched_tags->bitmap_tags.sb, m);
> -	mutex_unlock(&q->sysfs_lock);
> +	mutex_unlock(&q->elevator_lock);
>  
>  out:
>  	return res;
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 22600420799c..709a32022c78 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -568,9 +568,9 @@ struct request_queue {
>  	 * nr_requests and wbt latency, this lock also protects the sysfs attrs
>  	 * nr_requests and wbt_lat_usec. Additionally the nr_hw_queues update
>  	 * may modify hctx tags, reserved-tags and cpumask, so this lock also
> -	 * helps protect the hctx attrs. To ensure proper locking order during
> -	 * an elevator or nr_hw_queue update, first freeze the queue, then
> -	 * acquire ->elevator_lock.
> +	 * helps protect the hctx sysfs/debugfs attrs. To ensure proper locking
> +	 * order during an elevator or nr_hw_queue update, first freeze the
> +	 * queue, then acquire ->elevator_lock.
>  	 */
>  	struct mutex		elevator_lock;
>  


-- 
Damien Le Moal
Western Digital Research