On Fri, Feb 21, 2025 at 07:32:52PM +0530, 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. 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. Thanks, Ming
