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

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

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.
> 
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---

...

>  
> +#define QUEUE_RO_ENTRY_NOLOCK(_prefix, _name)			\
> +static struct queue_sysfs_entry _prefix##_entry = {		\
> +	.attr		= {.name = _name, .mode = 0644 },	\
> +	.show_nolock	= _prefix##_show,			\
> +}
> +
> +#define QUEUE_RW_ENTRY_NOLOCK(_prefix, _name)			\
> +static struct queue_sysfs_entry _prefix##_entry = {		\
> +	.attr		= {.name = _name, .mode = 0644 },	\
> +	.show_nolock	= _prefix##_show,			\
> +	.store_nolock	= _prefix##_store,			\
> +}
> +
>  #define QUEUE_RW_ENTRY(_prefix, _name)			\
>  static struct queue_sysfs_entry _prefix##_entry = {	\
>  	.attr	= { .name = _name, .mode = 0644 },	\
> @@ -446,7 +470,7 @@ QUEUE_RO_ENTRY(queue_max_discard_segments, "max_discard_segments");
>  QUEUE_RO_ENTRY(queue_discard_granularity, "discard_granularity");
>  QUEUE_RO_ENTRY(queue_max_hw_discard_sectors, "discard_max_hw_bytes");
>  QUEUE_LIM_RW_ENTRY(queue_max_discard_sectors, "discard_max_bytes");
> -QUEUE_RO_ENTRY(queue_discard_zeroes_data, "discard_zeroes_data");
> +QUEUE_RO_ENTRY_NOLOCK(queue_discard_zeroes_data, "discard_zeroes_data");

I think all QUEUE_RO_ENTRY needn't sysfs_lock, why do you just convert
part of them?


Thanks,
Ming