On 2/21/25 15:02, Nilay Shroff wrote:
Hi Christoph, Ming and others,
On 2/18/25 4:56 PM, Nilay Shroff wrote:
On 2/18/25 2:16 PM, Christoph Hellwig wrote:
On Tue, Feb 18, 2025 at 01:58:54PM +0530, Nilay Shroff wrote:
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.
Can you add the analys why they don't need sysfs_lock to this commit
message please?
Sure will do it in next patchset.
With that:
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
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.
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.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
