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 06:41:06PM +0530, Nilay Shroff wrote:
> 
> 
> On 2/18/25 5:40 PM, Ming Lei 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.
> >>
> >> 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?
> > 
> I think we have few read-only attributes which still need protection
> using q->limits_lock. So if you refer 2nd patch in the series then you'd
> find it. In this patch we group only attributes which don't require any
> locking and grouped them under show_nolock/store_nolock.

IMO, this RO attributes needn't protection from q->limits_lock:

- no lifetime issue

- in-tree code needn't limits_lock.

- all are scalar variable, so the attribute itself is updated atomically

- the limits still may be updated after lock is released

So what do you want to protect on these RO attributes? My concern is
that it is too complicated to maintain multiple versions of such macro.


Thanks,
Ming