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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2/22/25 6:14 PM, Ming Lei wrote:
> 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! So, as it seems we can't go wrong here if we don't use
READ_ONCE/WRITE_ONCE or read/write nr_zones without any lock, I'd 
send next patchset without any protection for nr_zones. Lets see 
if Christoph or others have any objection against this.

Thanks,
--Nilay







[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux