Re: [PATCHv2 1/6] blk-sysfs: remove q->sysfs_lock for attributes which don't need it

[Ming Lei <ming.lei@redhat.com>]

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@lst.de>
> >>
> > 
> 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