On 2/24/25 2:11 PM, Hannes Reinecke wrote: > 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. > Thanks Hannes! As you as well as Ming pointed out that it's unlikely to go wrong here with nr_zones even if we read/write it without any protection, I'd send next patchset without using READ_ONCE/WRITE_ONCE. Thanks, --Nilay
