[PATCHv2 0/3] block: protect debugfs attributes using elavtor_lock

[PATCHv2 0/3] block: protect debugfs attributes using elavtor_lock

[Nilay Shroff]

Hi,

This patchset contains total three patches.
The first patch helps protect debugfs attributes using q->elevator_lock
instead of q->sysfs_lock.
The second patch in the series removes the goto labels from the read
methods of debugfs attributes that improves code readability and reducing
complexity.
The third patch in the series protects debugfs attribute method hctx_
busy_show using q->elevator_lock.

Please note that this patchset was unit tested against blktests and quick
xfstests with lockdep enabled.
---
Changes from v1:
    - Split patch into smaller patches for bisectability. (hch)
    - Remove goto lable and return immediately upon failing to acquire
      the mutex lock. (Damien, hch)
    - Original patch in v1 is splitted into three patches
Link to v1: https://lore.kernel.org/all/20250312102903.3584358-1-nilay@linux.ibm.com/
---
Nilay Shroff (3):
  block: protect debugfs attrs using elevator_lock instead of sysfs_lock
  block: Remove unnecessary goto labels in debugfs attribute read
    methods
  block: protect debugfs attribute method hctx_busy_show

 block/blk-mq-debugfs.c | 41 +++++++++++++++++++++--------------------
 include/linux/blkdev.h |  6 +++---
 2 files changed, 24 insertions(+), 23 deletions(-)

-- 
2.47.1

[PATCHv2 1/3] block: protect debugfs attrs using elevator_lock instead of sysfs_lock

[Nilay Shroff]

Currently, the block debugfs attributes (tags, tags_bitmap, sched_tags,
and sched_tags_bitmap) are protected using q->sysfs_lock. However, these
attributes are updated in multiple scenarios:
- During driver probe method
- During an elevator switch/update
- During an nr_hw_queues update
- When writing to the sysfs attribute nr_requests

All these update paths (except driver probe method which anyways doesn't
not require any protection) are already protected using q->elevator_lock.
So to ensure consistency and proper synchronization, replace q->sysfs_
lock with q->elevator_lock for protecting these debugfs attributes.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 block/blk-mq-debugfs.c | 16 ++++++++--------
 include/linux/blkdev.h |  6 +++---
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index adf5f0697b6b..62775b132d4c 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -400,12 +400,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 +417,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 +434,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 +451,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;
 
-- 
2.47.1

Re: [PATCHv2 1/3] block: protect debugfs attrs using elevator_lock instead of sysfs_lock

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv2 2/3] block: Remove unnecessary goto labels in debugfs attribute read methods

[Nilay Shroff]

In some debugfs attribute read methods, failure to acquire the mutex lock
results in jumping to a label before returning an error code. However this
is unnecessary, as we can return the failure code directly, improving code
readability and reducing complexity.

This commit removes the goto labels and ensures that the method returns
immediately upon failing to acquire the mutex lock.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 block/blk-mq-debugfs.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 62775b132d4c..1c958bbaddce 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -402,13 +402,12 @@ static int hctx_tags_show(void *data, struct seq_file *m)
 
 	res = mutex_lock_interruptible(&q->elevator_lock);
 	if (res)
-		goto out;
+		return res;
 	if (hctx->tags)
 		blk_mq_debugfs_tags_show(m, hctx->tags);
 	mutex_unlock(&q->elevator_lock);
 
-out:
-	return res;
+	return 0;
 }
 
 static int hctx_tags_bitmap_show(void *data, struct seq_file *m)
@@ -419,13 +418,12 @@ static int hctx_tags_bitmap_show(void *data, struct seq_file *m)
 
 	res = mutex_lock_interruptible(&q->elevator_lock);
 	if (res)
-		goto out;
+		return res;
 	if (hctx->tags)
 		sbitmap_bitmap_show(&hctx->tags->bitmap_tags.sb, m);
 	mutex_unlock(&q->elevator_lock);
 
-out:
-	return res;
+	return 0;
 }
 
 static int hctx_sched_tags_show(void *data, struct seq_file *m)
@@ -436,13 +434,12 @@ static int hctx_sched_tags_show(void *data, struct seq_file *m)
 
 	res = mutex_lock_interruptible(&q->elevator_lock);
 	if (res)
-		goto out;
+		return res;
 	if (hctx->sched_tags)
 		blk_mq_debugfs_tags_show(m, hctx->sched_tags);
 	mutex_unlock(&q->elevator_lock);
 
-out:
-	return res;
+	return 0;
 }
 
 static int hctx_sched_tags_bitmap_show(void *data, struct seq_file *m)
@@ -453,13 +450,12 @@ static int hctx_sched_tags_bitmap_show(void *data, struct seq_file *m)
 
 	res = mutex_lock_interruptible(&q->elevator_lock);
 	if (res)
-		goto out;
+		return res;
 	if (hctx->sched_tags)
 		sbitmap_bitmap_show(&hctx->sched_tags->bitmap_tags.sb, m);
 	mutex_unlock(&q->elevator_lock);
 
-out:
-	return res;
+	return 0;
 }
 
 static int hctx_active_show(void *data, struct seq_file *m)
-- 
2.47.1

Re: [PATCHv2 2/3] block: Remove unnecessary goto labels in debugfs attribute read methods

[Christoph Hellwig]

Nit: the Subject starts with a capitalizes word after the prefix,
unlike the rest of the series and most block commits.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv2 3/3] block: protect debugfs attribute method hctx_busy_show

[Nilay Shroff]

The hctx_busy_show method in debugfs is currently unprotected.
This method iterates over all started requests in a tagset and
prints them. However, the tags can be updated concurrently via
the sysfs attributes 'nr_requests' or 'scheduler' (elevator
switch), leading to potential race conditions.

Since sysfs attributes 'nr_requests' and 'scheduler' are already
protected using q->elevator_lock, extend this protection to the
debugfs 'busy' attribute as well to ensure consistency.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 block/blk-mq-debugfs.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 1c958bbaddce..3421b5521fe2 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -347,9 +347,14 @@ 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)
+		return res;
 	blk_mq_tagset_busy_iter(hctx->queue->tag_set, hctx_show_busy_rq,
 				&params);
+	mutex_unlock(&hctx->queue->elevator_lock);
 
 	return 0;
 }
-- 
2.47.1

Re: [PATCHv2 3/3] block: protect debugfs attribute method hctx_busy_show

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv2 0/3] block: protect debugfs attributes using elavtor_lock

[Jens Axboe]

On Thu, 13 Mar 2025 17:21:49 +0530, Nilay Shroff wrote:
> This patchset contains total three patches.
> The first patch helps protect debugfs attributes using q->elevator_lock
> instead of q->sysfs_lock.
> The second patch in the series removes the goto labels from the read
> methods of debugfs attributes that improves code readability and reducing
> complexity.
> The third patch in the series protects debugfs attribute method hctx_
> busy_show using q->elevator_lock.
> 
> [...]

Applied, thanks!

[1/3] block: protect debugfs attrs using elevator_lock instead of sysfs_lock
      commit: a3996d11f3ab743e6cc4e3529ce9459c2cd27139
[2/3] block: Remove unnecessary goto labels in debugfs attribute read methods
      commit: 78800f5997d8ae0f20d4aced66a524f0f2fc4c7f
[3/3] block: protect debugfs attribute method hctx_busy_show
      commit: 0e94ed33681424a6dce65c62837e08e4c7aa09ac

Best regards,
-- 
Jens Axboe