block: fix lock order and remove redundant locking

There're few sysfs attributes in block layer which don't really need acquiring q->sysfs_lock while accessing it. The reason being, writing a value to such attributes are either atomic or could be easily protected using WRITE_ONCE()/READ_ONCE(). Moreover, sysfs attributes are inherently protected with sysfs/kernfs internal locking.

So this change help segregate all existing sysfs attributes for which we could avoid acquiring q->sysfs_lock. We group all such attributes, which don't require any sorts of locking, using macro QUEUE_RO_ENTRY_ NOLOCK() or QUEUE_RW_ENTRY_NOLOCK(). The newly introduced show/store method (show_nolock/store_nolock) is assigned to attributes using these new macros. The show_nolock/store_nolock run without holding q->sysfs_ lock.

Quoted 6 lines · Nilay Shroff

There're few sysfs attributes in block layer which don't really need acquiring q->sysfs_lock while accessing it. The reason being, writing a value to such attributes are either atomic or could be easily protected using WRITE_ONCE()/READ_ONCE(). Moreover, sysfs attributes are inherently protected with sysfs/kernfs internal locking.

Quoted 7 lines

So this change help segregate all existing sysfs attributes for which we could avoid acquiring q->sysfs_lock. We group all such attributes, which don't require any sorts of locking, using macro QUEUE_RO_ENTRY_ NOLOCK() or QUEUE_RW_ENTRY_NOLOCK(). The newly introduced show/store method (show_nolock/store_nolock) is assigned to attributes using these new macros. The show_nolock/store_nolock run without holding q->sysfs_ lock.

Can you add the analys why they don't need sysfs_lock to this commit message please?

With that:

Reviewed-by Christoph Hellwig <[email protected]>

Quoted 15 lines · Christoph Hellwig

Quoted 13 lines · Nilay Shroff

There're few sysfs attributes in block layer which don't really need acquiring q->sysfs_lock while accessing it. The reason being, writing a value to such attributes are either atomic or could be easily protected using WRITE_ONCE()/READ_ONCE(). Moreover, sysfs attributes are inherently protected with sysfs/kernfs internal locking.

So this change help segregate all existing sysfs attributes for which we could avoid acquiring q->sysfs_lock. We group all such attributes, which don't require any sorts of locking, using macro QUEUE_RO_ENTRY_ NOLOCK() or QUEUE_RW_ENTRY_NOLOCK(). The newly introduced show/store method (show_nolock/store_nolock) is assigned to attributes using these new macros. The show_nolock/store_nolock run without holding q->sysfs_ lock.

Quoted 2 lines

Can you add the analys why they don't need sysfs_lock to this commit message please?

Sure will do it in next patchset.

Quoted 5 lines

With that:

Reviewed-by Christoph Hellwig <[email protected]>

Hi Christoph, Ming and others,

Quoted 18 lines · Nilay Shroff

Quoted 15 lines · Christoph Hellwig

Quoted 13 lines · Nilay Shroff

There're few sysfs attributes in block layer which don't really need acquiring q->sysfs_lock while accessing it. The reason being, writing a value to such attributes are either atomic or could be easily protected using WRITE_ONCE()/READ_ONCE(). Moreover, sysfs attributes are inherently protected with sysfs/kernfs internal locking.

So this change help segregate all existing sysfs attributes for which we could avoid acquiring q->sysfs_lock. We group all such attributes, which don't require any sorts of locking, using macro QUEUE_RO_ENTRY_ NOLOCK() or QUEUE_RW_ENTRY_NOLOCK(). The newly introduced show/store method (show_nolock/store_nolock) is assigned to attributes using these new macros. The show_nolock/store_nolock run without holding q->sysfs_ lock.

Quoted 9 lines

Quoted 2 lines

Can you add the analys why they don't need sysfs_lock to this commit message please?

Sure will do it in next patchset.

Quoted 5 lines

With that:

Reviewed-by Christoph Hellwig <[email protected]>

I think we discussed about all attributes which don't require locking, however there's one which I was looking at "nr_zones" which we haven't discussed. This is read-only attribute and currently protected with q->sysfs_lock.

Write to this attribute (nr_zones) mostly happens in the driver probe method (except nvme) before disk is added and outside of q->sysfs_lock or any other lock. But in case of nvme it could be updated from disk scan.
nvme_validate_ns
-> nvme_update_ns_info_block

    -> blk_revalidate_disk_zones
      -> disk_update_zone_resources

The update to disk->nr_zones is done outside of queue freeze or any other lock today. So do you agree if we could use READ_ONCE/WRITE_ONCE to protect this attribute and remove q->sysfs_lock? I think, it'd be great if we could agree upon this one before I send the next patchset.

Quoted 42 lines · Nilay Shroff

Hi Christoph, Ming and others,

Quoted 18 lines · Nilay Shroff

Quoted 15 lines · Christoph Hellwig

Quoted 13 lines · Nilay Shroff

There're few sysfs attributes in block layer which don't really need acquiring q->sysfs_lock while accessing it. The reason being, writing a value to such attributes are either atomic or could be easily protected using WRITE_ONCE()/READ_ONCE(). Moreover, sysfs attributes are inherently protected with sysfs/kernfs internal locking.

So this change help segregate all existing sysfs attributes for which we could avoid acquiring q->sysfs_lock. We group all such attributes, which don't require any sorts of locking, using macro QUEUE_RO_ENTRY_ NOLOCK() or QUEUE_RW_ENTRY_NOLOCK(). The newly introduced show/store method (show_nolock/store_nolock) is assigned to attributes using these new macros. The show_nolock/store_nolock run without holding q->sysfs_ lock.

Quoted 9 lines

Quoted 2 lines

Can you add the analys why they don't need sysfs_lock to this commit message please?

Sure will do it in next patchset.

Quoted 5 lines

With that:

Reviewed-by Christoph Hellwig <[email protected]>

I think we discussed about all attributes which don't require locking, however there's one which I was looking at "nr_zones" which we haven't discussed. This is read-only attribute and currently protected with q->sysfs_lock.

Write to this attribute (nr_zones) mostly happens in the driver probe method (except nvme) before disk is added and outside of q->sysfs_lock or any other lock. But in case of nvme it could be updated from disk scan.
nvme_validate_ns
-> nvme_update_ns_info_block

Quoted 7 lines

    -> blk_revalidate_disk_zones
      -> disk_update_zone_resources

The update to disk->nr_zones is done outside of queue freeze or any other lock today. So do you agree if we could use READ_ONCE/WRITE_ONCE to protect this attribute and remove q->sysfs_lock? I think, it'd be great if we could agree upon this one before I send the next patchset.

IMO, it is fine to read it lockless without READ_ONCE/WRITE_ONCE because disk->nr_zones is defined as 'unsigned int', which won't return garbage value anytime.

But I don't object if you want to change to READ_ONCE/WRITE_ONCE.

IMO, it is fine to read it lockless without READ_ONCE/WRITE_ONCE because disk->nr_zones is defined as 'unsigned int', which won't return garbage value anytime.

But I don't object if you want to change to READ_ONCE/WRITE_ONCE.

Quoted 5 lines · Ming Lei

IMO, it is fine to read it lockless without READ_ONCE/WRITE_ONCE because disk->nr_zones is defined as 'unsigned int', which won't return garbage value anytime.

But I don't object if you want to change to READ_ONCE/WRITE_ONCE.

It changes every time the disk capacity changes. And on the (uncommon) reformats. So the best locking would be the same as for the disk capacity.

Hi Christoph, Ming and others,

Quoted 18 lines · Nilay Shroff

Quoted 15 lines · Christoph Hellwig

Quoted 13 lines · Nilay Shroff

There're few sysfs attributes in block layer which don't really need acquiring q->sysfs_lock while accessing it. The reason being, writing a value to such attributes are either atomic or could be easily protected using WRITE_ONCE()/READ_ONCE(). Moreover, sysfs attributes are inherently protected with sysfs/kernfs internal locking.

So this change help segregate all existing sysfs attributes for which we could avoid acquiring q->sysfs_lock. We group all such attributes, which don't require any sorts of locking, using macro QUEUE_RO_ENTRY_ NOLOCK() or QUEUE_RW_ENTRY_NOLOCK(). The newly introduced show/store method (show_nolock/store_nolock) is assigned to attributes using these new macros. The show_nolock/store_nolock run without holding q->sysfs_ lock.

Quoted 9 lines

Quoted 2 lines

Can you add the analys why they don't need sysfs_lock to this commit message please?

Sure will do it in next patchset.

Quoted 5 lines

With that:

Reviewed-by Christoph Hellwig <[email protected]>

I think we discussed about all attributes which don't require locking, however there's one which I was looking at "nr_zones" which we haven't discussed. This is read-only attribute and currently protected with q->sysfs_lock.

Write to this attribute (nr_zones) mostly happens in the driver probe method (except nvme) before disk is added and outside of q->sysfs_lock or any other lock. But in case of nvme it could be updated from disk scan.
nvme_validate_ns
-> nvme_update_ns_info_block

     -> blk_revalidate_disk_zones
       -> disk_update_zone_resources

The update to disk->nr_zones is done outside of queue freeze or any other lock today. So do you agree if we could use READ_ONCE/WRITE_ONCE to protect this attribute and remove q->sysfs_lock? I think, it'd be great if we could agree upon this one before I send the next patchset.

Quoted 45 lines · Nilay Shroff

Hi Christoph, Ming and others,

Quoted 18 lines · Nilay Shroff

Quoted 15 lines · Christoph Hellwig

Quoted 13 lines · Nilay Shroff

There're few sysfs attributes in block layer which don't really need acquiring q->sysfs_lock while accessing it. The reason being, writing a value to such attributes are either atomic or could be easily protected using WRITE_ONCE()/READ_ONCE(). Moreover, sysfs attributes are inherently protected with sysfs/kernfs internal locking.

So this change help segregate all existing sysfs attributes for which we could avoid acquiring q->sysfs_lock. We group all such attributes, which don't require any sorts of locking, using macro QUEUE_RO_ENTRY_ NOLOCK() or QUEUE_RW_ENTRY_NOLOCK(). The newly introduced show/store method (show_nolock/store_nolock) is assigned to attributes using these new macros. The show_nolock/store_nolock run without holding q->sysfs_ lock.

Quoted 9 lines

Quoted 2 lines

Can you add the analys why they don't need sysfs_lock to this commit message please?

Sure will do it in next patchset.

Quoted 5 lines

With that:

Reviewed-by Christoph Hellwig <[email protected]>

I think we discussed about all attributes which don't require locking, however there's one which I was looking at "nr_zones" which we haven't discussed. This is read-only attribute and currently protected with q->sysfs_lock.

Write to this attribute (nr_zones) mostly happens in the driver probe method (except nvme) before disk is added and outside of q->sysfs_lock or any other lock. But in case of nvme it could be updated from disk scan.
nvme_validate_ns
   -> nvme_update_ns_info_block
     -> blk_revalidate_disk_zones
       -> disk_update_zone_resources

Quoted 5 lines

The update to disk->nr_zones is done outside of queue freeze or any other lock today. So do you agree if we could use READ_ONCE/WRITE_ONCE to protect this attribute and remove q->sysfs_lock? I think, it'd be great if we could agree upon this one before I send the next patchset.

READ_ONCE should be fine here. 'nr_zones' is unlikely to change, and if that is updated we've done a full disk revalidation including a read of all zones. So not a critical operation, and nothing which needs to be protected.

There're few sysfs attributes in block layer which don't really need acquiring q->sysfs_lock while accessing it. The reason being, writing a value to such attributes are either atomic or could be easily protected using WRITE_ONCE()/READ_ONCE(). Moreover, sysfs attributes are inherently protected with sysfs/kernfs internal locking.

So this change help segregate all existing sysfs attributes for which we could avoid acquiring q->sysfs_lock. We group all such attributes, which don't require any sorts of locking, using macro QUEUE_RO_ENTRY_ NOLOCK() or QUEUE_RW_ENTRY_NOLOCK(). The newly introduced show/store method (show_nolock/store_nolock) is assigned to attributes using these new macros. The show_nolock/store_nolock run without holding q->sysfs_ lock.

Signed-off-by Nilay Shroff <[email protected]>

---

block/blk-sysfs.c +13 -0

+#define QUEUE_RO_ENTRY_NOLOCK(_prefix, _name)			\
+static struct queue_sysfs_entry _prefix##_entry = {		\
+	.attr		= {.name = _name, .mode = 0644 },	\
+	.show_nolock	= _prefix##_show,			\
+}
+
+#define QUEUE_RW_ENTRY_NOLOCK(_prefix, _name)			\
+static struct queue_sysfs_entry _prefix##_entry = {		\
+	.attr		= {.name = _name, .mode = 0644 },	\
+	.show_nolock	= _prefix##_show,			\
+	.store_nolock	= _prefix##_store,			\
+}
+
 #define QUEUE_RW_ENTRY(_prefix, _name)			\
 static struct queue_sysfs_entry _prefix##_entry = {	\
 	.attr	= { .name = _name, .mode = 0644 },	\

Quoted 16 lines · Nilay Shroff

There're few sysfs attributes in block layer which don't really need acquiring q->sysfs_lock while accessing it. The reason being, writing a value to such attributes are either atomic or could be easily protected using WRITE_ONCE()/READ_ONCE(). Moreover, sysfs attributes are inherently protected with sysfs/kernfs internal locking.

So this change help segregate all existing sysfs attributes for which we could avoid acquiring q->sysfs_lock. We group all such attributes, which don't require any sorts of locking, using macro QUEUE_RO_ENTRY_ NOLOCK() or QUEUE_RW_ENTRY_NOLOCK(). The newly introduced show/store method (show_nolock/store_nolock) is assigned to attributes using these new macros. The show_nolock/store_nolock run without holding q->sysfs_ lock.

Signed-off-by Nilay Shroff <[email protected]>

---

...

Quoted 18 lines

block/blk-sysfs.c +13 -0

+#define QUEUE_RO_ENTRY_NOLOCK(_prefix, _name)			\
+static struct queue_sysfs_entry _prefix##_entry = {		\
+	.attr		= {.name = _name, .mode = 0644 },	\
+	.show_nolock	= _prefix##_show,			\
+}
+
+#define QUEUE_RW_ENTRY_NOLOCK(_prefix, _name)			\
+static struct queue_sysfs_entry _prefix##_entry = {		\
+	.attr		= {.name = _name, .mode = 0644 },	\
+	.show_nolock	= _prefix##_show,			\
+	.store_nolock	= _prefix##_store,			\
+}
+
 #define QUEUE_RW_ENTRY(_prefix, _name)			\
 static struct queue_sysfs_entry _prefix##_entry = {	\
 	.attr	= { .name = _name, .mode = 0644 },	\

@@ -446,7 +470,7 @@ QUEUE_RO_ENTRY(queue_max_discard_segments, "max_discard_segments");

block/blk-sysfs.c QUEUE_RO_ENTRY(queue_max_discard_segments, "max_discard_segments"); · line 473

Quoted 5 lines

block/blk-sysfs.c +1 -1

 QUEUE_RO_ENTRY(queue_discard_granularity, "discard_granularity");
 QUEUE_RO_ENTRY(queue_max_hw_discard_sectors, "discard_max_hw_bytes");
 QUEUE_LIM_RW_ENTRY(queue_max_discard_sectors, "discard_max_bytes");
-QUEUE_RO_ENTRY(queue_discard_zeroes_data, "discard_zeroes_data");
+QUEUE_RO_ENTRY_NOLOCK(queue_discard_zeroes_data, "discard_zeroes_data");

I think all QUEUE_RO_ENTRY needn't sysfs_lock, why do you just convert part of them?

Quoted 38 lines · Ming Lei

Quoted 16 lines · Nilay Shroff

There're few sysfs attributes in block layer which don't really need acquiring q->sysfs_lock while accessing it. The reason being, writing a value to such attributes are either atomic or could be easily protected using WRITE_ONCE()/READ_ONCE(). Moreover, sysfs attributes are inherently protected with sysfs/kernfs internal locking.

So this change help segregate all existing sysfs attributes for which we could avoid acquiring q->sysfs_lock. We group all such attributes, which don't require any sorts of locking, using macro QUEUE_RO_ENTRY_ NOLOCK() or QUEUE_RW_ENTRY_NOLOCK(). The newly introduced show/store method (show_nolock/store_nolock) is assigned to attributes using these new macros. The show_nolock/store_nolock run without holding q->sysfs_ lock.

Signed-off-by Nilay Shroff <[email protected]>

---

...

Quoted 18 lines

block/blk-sysfs.c +13 -0

+#define QUEUE_RO_ENTRY_NOLOCK(_prefix, _name)			\
+static struct queue_sysfs_entry _prefix##_entry = {		\
+	.attr		= {.name = _name, .mode = 0644 },	\
+	.show_nolock	= _prefix##_show,			\
+}
+
+#define QUEUE_RW_ENTRY_NOLOCK(_prefix, _name)			\
+static struct queue_sysfs_entry _prefix##_entry = {		\
+	.attr		= {.name = _name, .mode = 0644 },	\
+	.show_nolock	= _prefix##_show,			\
+	.store_nolock	= _prefix##_store,			\
+}
+
 #define QUEUE_RW_ENTRY(_prefix, _name)			\
 static struct queue_sysfs_entry _prefix##_entry = {	\
 	.attr	= { .name = _name, .mode = 0644 },	\

@@ -446,7 +470,7 @@ QUEUE_RO_ENTRY(queue_max_discard_segments, "max_discard_segments");

Quoted 9 lines

block/blk-sysfs.c QUEUE_RO_ENTRY(queue_max_discard_segments, "max_discard_segments"); · line 473

Quoted 5 lines

block/blk-sysfs.c +1 -1

 QUEUE_RO_ENTRY(queue_discard_granularity, "discard_granularity");
 QUEUE_RO_ENTRY(queue_max_hw_discard_sectors, "discard_max_hw_bytes");
 QUEUE_LIM_RW_ENTRY(queue_max_discard_sectors, "discard_max_bytes");
-QUEUE_RO_ENTRY(queue_discard_zeroes_data, "discard_zeroes_data");
+QUEUE_RO_ENTRY_NOLOCK(queue_discard_zeroes_data, "discard_zeroes_data");

I think all QUEUE_RO_ENTRY needn't sysfs_lock, why do you just convert part of them?

I think we have few read-only attributes which still need protection using q->limits_lock. So if you refer 2nd patch in the series then you'd find it. In this patch we group only attributes which don't require any locking and grouped them under show_nolock/store_nolock.

IMO, this RO attributes needn't protection from q->limits_lock:

- no lifetime issue

- in-tree code needn't limits_lock.

- all are scalar variable, so the attribute itself is updated atomically

Quoted 7 lines · Ming Lei

IMO, this RO attributes needn't protection from q->limits_lock:

- no lifetime issue

- in-tree code needn't limits_lock.

- all are scalar variable, so the attribute itself is updated atomically

Except in the memory model they aren't without READ_ONCE/WRITE_ONCE.

Given that the limits_lock is not a hot lock taking the lock is a very easy way to mark our intent. And if we get things like thread thread sanitizer patches merged that will become essential. Even KCSAN might object already without it.

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Ming Lei

IMO, this RO attributes needn't protection from q->limits_lock:

- no lifetime issue

Quoted 5 lines

Quoted 3 lines

- in-tree code needn't limits_lock.

- all are scalar variable, so the attribute itself is updated atomically

Except in the memory model they aren't without READ_ONCE/WRITE_ONCE.

RW_ONCE is supposed for avoiding compiler optimization, and scalar variable atomic update should be decided by hardware.

Quoted 10 lines · Christoph Hellwig

Quoted 7 lines · Ming Lei

IMO, this RO attributes needn't protection from q->limits_lock:

- no lifetime issue

- in-tree code needn't limits_lock.

- all are scalar variable, so the attribute itself is updated atomically

Except in the memory model they aren't without READ_ONCE/WRITE_ONCE.

RW_ONCE is supposed for avoiding compiler optimization, and scalar variable atomic update should be decided by hardware.

Quoted 1 line

Quoted 4 lines

Given that the limits_lock is not a hot lock taking the lock is a very easy way to mark our intent. And if we get things like thread thread sanitizer patches merged that will become essential. Even KCSAN might object already without it.

My main concern is that there are too many ->store()/->load() variants now, but not deal if you think this way is fine, :-)

Quoted 21 lines · Ming Lei

Quoted 10 lines · Christoph Hellwig

Quoted 7 lines · Ming Lei

IMO, this RO attributes needn't protection from q->limits_lock:

- no lifetime issue

- in-tree code needn't limits_lock.

- all are scalar variable, so the attribute itself is updated atomically

Except in the memory model they aren't without READ_ONCE/WRITE_ONCE.

RW_ONCE is supposed for avoiding compiler optimization, and scalar variable atomic update should be decided by hardware.

Quoted 5 lines

Given that the limits_lock is not a hot lock taking the lock is a very easy way to mark our intent. And if we get things like thread thread sanitizer patches merged that will become essential. Even KCSAN might object already without it.

Quoted 3 lines

My main concern is that there are too many ->store()/->load() variants now, but not deal if you think this way is fine, :-)

We will only have ->store_limit()/->show_limit() and ->store()/->load() in
the next patchset as I am going to cleanup load_module() as well as get away with show_nolock() and store_nolock() methods as discussed with Christoph in another thread.

Quoted 24 lines · Nilay Shroff

Quoted 21 lines · Ming Lei

Quoted 10 lines · Christoph Hellwig

Quoted 7 lines · Ming Lei

IMO, this RO attributes needn't protection from q->limits_lock:

- no lifetime issue

- in-tree code needn't limits_lock.

- all are scalar variable, so the attribute itself is updated atomically

Except in the memory model they aren't without READ_ONCE/WRITE_ONCE.

RW_ONCE is supposed for avoiding compiler optimization, and scalar variable atomic update should be decided by hardware.

Quoted 5 lines

Given that the limits_lock is not a hot lock taking the lock is a very easy way to mark our intent. And if we get things like thread thread sanitizer patches merged that will become essential. Even KCSAN might object already without it.

Quoted 7 lines

Quoted 3 lines

My main concern is that there are too many ->store()/->load() variants now, but not deal if you think this way is fine, :-)

We will only have ->store_limit()/->show_limit() and ->store()/->load() in
the next patchset as I am going to cleanup load_module() as well as get away with show_nolock() and store_nolock() methods as discussed with Christoph in another thread.

Sorry a typo, I meant we will only have ->store_limit()/->show_limit() and ->store()/show() methods. Also, we'll cleanup load_module() as well as get away with show_nolock() and store_nolock() methods in the next patchset as discussed with Christoph in another thread.

As the scope of q->sysfs_lock is not well-defined, its misuse has resulted in numerous lockdep warnings. To resolve this, replace q-> sysfs_lock with a new dedicated q->elevator_lock, which will be exclusively used to protect elevator switching and updates.

/* the check has to be done with holding sysfs_lock */

Quoted 4 lines · Nilay Shroff

As the scope of q->sysfs_lock is not well-defined, its misuse has resulted in numerous lockdep warnings. To resolve this, replace q-> sysfs_lock with a new dedicated q->elevator_lock, which will be exclusively used to protect elevator switching and updates.

Well, it's not replacing q->sysfs_lock as that is still around. It changes some data to now be protected by elevator_lock instead, and you should spell out which, preferably in a comment next to the elevator_lock definition (I should have done the same for limits_lock despite the trivial applicability to the limits field).

block/blk-mq.c static bool blk_mq_elv_switch_none(struct list_head *head, · line 4937

Quoted 3 lines

block/blk-mq.c +1 -1

 	/* q->elevator needs protection from ->sysfs_lock */
-	mutex_lock(&q->sysfs_lock);
+	mutex_lock(&q->elevator_lock);

Well, the above comment is no trivially wrong.

Quoted 3 lines

/* the check has to be done with holding sysfs_lock */

Same for this one.

They could probably go away with the proper comments near elevator_lock itself.

block/blk-sysfs.c queue_var_store(unsigned long *var, const char *page, size_t count) · line 65

Quoted 9 lines

static ssize_t queue_requests_show(struct gendisk *disk, char *page) {

block/blk-sysfs.c +5 -1

-	return queue_var_show(disk->queue->nr_requests, page);
+	int ret;
+
+	mutex_lock(&disk->queue->sysfs_lock);
+	ret = queue_var_show(disk->queue->nr_requests, page);
+	mutex_unlock(&disk->queue->sysfs_lock);

This also shifts taking sysfs_lock into the the ->show and ->store methods. Which feels like a good thing, but isn't mentioned in the commit log or directly releate to elevator_lock. Can you please move taking the locking into the methods into a preparation patch with a proper commit log?

Also it's pretty strange the ->store_nolock is
now called with the queue frozen but ->store is not and both are called without any locks. So I think part of that prep patch should also be moving the queue freezing into ->store and do away with the separate ->store_nolock, and just keep the special ->store_limit.

There's also no more need for ->lock_module when the elevator store method is called unfrozen and without a lock.

->show and ->show_nolock also are identical now and can be done away with.

So maybe shifting the freezing and locking into the methods should go very first, before dropping the lock for the attributes that don't it?

So now if you suggest keeping only ->store and do away with ->store_nolock method then we have to move feeze-lock inside each attributes' corresponding store method as we can't keep freeze-lock under ->store in queue_attr_store().

Quoted 3 lines · Nilay Shroff

So now if you suggest keeping only ->store and do away with ->store_nolock method then we have to move feeze-lock inside each attributes' corresponding store method as we can't keep freeze-lock under ->store in queue_attr_store().

Yes, that's what I suggested in my previous mail:

"So I think part of that prep patch should
also be moving the queue freezing into ->store and do away with the separate ->store_nolock"

block/blk-sysfs.c queue_ra_store(struct gendisk *disk, const char *page, size_t count) · line 129

Quoted 8 lines

block/blk-sysfs.c +8 -0

+	/*
+	 * We don't use atomic update helper queue_limits_start_update() and
+	 * queue_limits_commit_update() here for updaing ra_pages bacause
+	 * blk_apply_bdi_limits() which is invoked from  queue_limits_commit_
+	 * update() can overwrite the ra_pages value which user actaully wants
+	 * to store here. The blk_apply_bdi_limits() sets value of ra_pages
+	 * based on the optimal I/O size(io_opt).
+	 */

Maybe replace this with:

	/*
	 * ra_pages is protected by limit_lock because it is usually
	 * calculated from the queue limits by queue_limits_commit_update.
	 */

The mix of blk-sysfs and block in the subject lines is a bit odd. Maybe just use the block prefix everywhere?

Also q->sysfs_lock is almost unused now and we should probably look into killing it entirely.

blk_mq_hw_sysfs_show takes it around the ->show methods which looks pretty useless. The debugfs code takes it for a few undocumented things, which are worth digging into and if needed split into a separate lock.

The concurrent ranges code takes it - I think that is because it does register a complex sysfs hierarchy from something that could race with add_disk / del_gendisk. Damien, can you help with your thoughts? (sd.c also has a comment reference it and the removed sysfs_dir_lock which needs fixing anyway).

blk_register_queue still takes it around a pretty random range of code including nesting with other locks. I can't see what it protects against, but it could use a careful look.

blk_unregister_queue takes it just to clear QUEUE_FLAG_REGISTERED, which by definition can't really protect against anything.

Also the sysfs_lock in the elevator_queue should probably go away or be replaced with the new elevator_lock for the non-show/store path for the same reasons as outlined in this series.