Re: [PATCH 12/15] block: move debugfs/sysfs register out of freezing queue

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

On Fri, Apr 11, 2025 at 12:27:17AM +0530, Nilay Shroff wrote:
> 
> 
> On 4/10/25 7:00 PM, Ming Lei wrote:
> > Move debugfs/sysfs register out of freezing queue in
> > __blk_mq_update_nr_hw_queues(), so that the following lockdep dependency
> > can be killed:
> > 
> > 	#2 (&q->q_usage_counter(io)#16){++++}-{0:0}:
> > 	#1 (fs_reclaim){+.+.}-{0:0}:
> > 	#0 (&sb->s_type->i_mutex_key#3){+.+.}-{4:4}: //debugfs
> > 
> > And registering/un-registering debugfs/sysfs does not require queue to be
> > frozen.
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/blk-mq.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index 7219b01764da..0fb72a698d77 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -4947,15 +4947,15 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
> >  	if (set->nr_maps == 1 && nr_hw_queues == set->nr_hw_queues)
> >  		return;
> >  
> > -	memflags = memalloc_noio_save();
> > -	list_for_each_entry(q, &set->tag_list, tag_set_list)
> > -		blk_mq_freeze_queue_nomemsave(q);
> > -
> >  	list_for_each_entry(q, &set->tag_list, tag_set_list) {
> >  		blk_mq_debugfs_unregister_hctxs(q);
> >  		blk_mq_sysfs_unregister_hctxs(q);
> >  	}
> As we removed hctx sysfs protection while un-registering it, this might
> cause crash or other side-effect if simultaneously these sysfs attributes
> are accessed. The read access of these attributes are still protected 
> using ->elevator_lock. 

The ->elevator_lock in ->show() is useless except for reading the elevator
internal data(sched tags, requests, ...), even for reading elevator data,
it should have been relying on elevator reference, instead of lock, but
that is another topic & improvement in future.

Also this patch does _not_ change ->elevator_lock for above debugfs/sysfs
unregistering, does it? It is always done without holding ->elevator_lock.
Also ->show() does not require ->q_usage_counter too.

As I mentioned, kobject/sysfs provides protection between ->show()/->store()
and kobject_del(), isn't it the reason why you want to remove ->sys_lock?

https://lore.kernel.org/linux-block/20250226124006.1593985-1-nilay@linux.ibm.com/


Thanks,
Ming