[2/2] block: avoid acquiring q->sysfs_lock while accessing sysfs attributes

Commit Message

Nilay Shroff Feb. 5, 2025, 2:44 p.m. UTC

The sysfs attributes are already protected with sysfs/kernfs internal
locking. So acquiring q->sysfs_lock is not needed while accessing sysfs
attribute files. So this change helps avoid holding q->sysfs_lock while
accessing sysfs attribute files.

Signed-off-by: Nilay Shroff <[e-mail]>
---
 block/blk-mq-sysfs.c |  6 +-----
 block/blk-sysfs.c    | 10 +++-------
 2 files changed, 4 insertions(+), 12 deletions(-)

Comments

Christoph Hellwig Feb. 5, 2025, 3:53 p.m. UTC | #1

On Wed, Feb 05, 2025 at 08:14:48PM +0530, Nilay Shroff wrote:
> The sysfs attributes are already protected with sysfs/kernfs internal
> locking. So acquiring q->sysfs_lock is not needed while accessing sysfs
> attribute files. So this change helps avoid holding q->sysfs_lock while
> accessing sysfs attribute files.

the sysfs/kernfs locking only protects against other accesses using
sysfs.  But that's not really the most interesting part here.  We
also want to make sure nothing changes underneath in a way that
could cause crashes (and maybe even torn information).

We'll really need to audit what is accessed in each method and figure
out what protects it.  Chances are that sysfs_lock provides that
protection in some case right now, and chances are also very high
that a lot of this is pretty broken.

Patch

diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
index 3feeeccf8a99..da53397d99fa 100644
--- a/block/blk-mq-sysfs.c
+++ b/block/blk-mq-sysfs.c
@@ -52,7 +52,6 @@  static ssize_t blk_mq_hw_sysfs_show(struct kobject *kobj,
 	struct blk_mq_hw_ctx_sysfs_entry *entry;
 	struct blk_mq_hw_ctx *hctx;
 	struct request_queue *q;
-	ssize_t res;
 
 	entry = container_of(attr, struct blk_mq_hw_ctx_sysfs_entry, attr);
 	hctx = container_of(kobj, struct blk_mq_hw_ctx, kobj);
@@ -61,10 +60,7 @@  static ssize_t blk_mq_hw_sysfs_show(struct kobject *kobj,
 	if (!entry->show)
 		return -EIO;
 
-	mutex_lock(&q->sysfs_lock);
-	res = entry->show(hctx, page);
-	mutex_unlock(&q->sysfs_lock);
-	return res;
+	return entry->show(hctx, page);
 }
 
 static ssize_t blk_mq_hw_sysfs_nr_tags_show(struct blk_mq_hw_ctx *hctx,
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 6f548a4376aa..2b8e7b311c61 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -664,14 +664,11 @@  queue_attr_show(struct kobject *kobj, struct attribute *attr, char *page)
 {
 	struct queue_sysfs_entry *entry = to_queue(attr);
 	struct gendisk *disk = container_of(kobj, struct gendisk, queue_kobj);
-	ssize_t res;
 
 	if (!entry->show)
 		return -EIO;
-	mutex_lock(&disk->queue->sysfs_lock);
-	res = entry->show(disk, page);
-	mutex_unlock(&disk->queue->sysfs_lock);
-	return res;
+
+	return entry->show(disk, page);
 }
 
 static ssize_t
@@ -710,11 +707,10 @@  queue_attr_store(struct kobject *kobj, struct attribute *attr,
 		return length;
 	}
 
-	mutex_lock(&q->sysfs_lock);
 	memflags = blk_mq_freeze_queue(q);
 	res = entry->store(disk, page, length);
 	blk_mq_unfreeze_queue(q, memflags);
-	mutex_unlock(&q->sysfs_lock);
+
 	return res;
 }