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

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

On Wed, Feb 19, 2025 at 02:26:49PM +0530, Nilay Shroff wrote:
> 
> 
> On 2/19/25 2:04 PM, Nilay Shroff wrote:
> > 
> > 
> > On 2/19/25 8:54 AM, Ming Lei wrote:
> >> On Tue, Feb 18, 2025 at 05:29:53PM +0100, Christoph Hellwig wrote:
> >>> On Tue, Feb 18, 2025 at 09:45:02PM +0800, Ming Lei wrote:
> >>>> 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
> >>>
> >>> Except in the memory model they aren't without READ_ONCE/WRITE_ONCE.
> >>
> >> RW_ONCE is supposed for avoiding compiler optimization, and scalar
> >> variable atomic update should be decided by hardware.
> >>
> >>>
> >>> Given that the limits_lock is not a hot lock taking the lock is a very
> >>> easy way to mark our intent.  And if we get things like thread thread
> >>> sanitizer patches merged that will become essential.  Even KCSAN
> >>> might object already without it.
> >>
> >> My main concern is that there are too many ->store()/->load() variants
> >> now, but not deal if you think this way is fine, :-)
> >>
> > We will only have ->store_limit()/->show_limit() and ->store()/->load() in
> > the next patchset as I am going to cleanup load_module() as well as get away with show_nolock() and store_nolock() methods as discussed with Christoph in 
> > another thread.
> > 
> 
> Sorry a typo, I meant we will only have ->store_limit()/->show_limit()
> and ->store()/show() methods. Also, we'll cleanup load_module() as well
> as get away with show_nolock() and store_nolock() methods in the next 
> patchset as discussed with Christoph in another thread.

OK, that looks much better!


Thanks,
Ming