On 3/12/25 4:21 PM, Damien Le Moal wrote:
> 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,
>> ¶ms);
>> -
>> - return 0;
>> + mutex_unlock(&hctx->queue->elevator_lock);
>> +out:
>> + return res;
>
> And you can keep the "return 0;" here.
>
Yes I agree. And in fact, that's how exactly I initially prepared the patch
however later I saw that other places in this file where we use mutex_lock_
interruptible(), we use goto when acquiring the lock fails and so I changed
it here to match those other occurrences. So if everyone prefer, shall I use
your suggested pattern at other places as well in this file?
>>
>>
>> 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;
>>
Thanks,
--Nilay
