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

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

On Tue, Apr 15, 2025 at 04:45:16PM +0530, Nilay Shroff wrote:
> 
> 
> On 4/15/25 3:36 PM, Ming Lei wrote:
> > On Tue, Apr 15, 2025 at 03:07:18PM +0530, Nilay Shroff wrote:
> >>
> >>
> >> On 4/14/25 7:12 AM, Ming Lei wrote:
> >>> 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/
> >>>
> >> Yes you were correct, that was the reason we wanted to remove ->sysfs_lock.
> >> However for these particular hctx sysfs attributes (nr_tags and nr_reserved_tags)
> >> could be updated simultaneously from another blk-mq sysfs attribute named nr_requests.
> >> Hence IMO, the default protection provided by sysfs/kernfs may not be sufficient and
> >> so we need to protect those attributes using ->elevator_lock.
> > 
> > Yes, what is why this patchset doesn't kill more ->elevator_lock uses, such
> > as, the uses in blk-mq-debugs, update_nr_requests, but many of them can be
> > replaced with grabbing elevator reference.
> > 
> > But with/without this patch, the touched register/unregisger code does not
> > require ->elevator_lock:
> > 
> >                 blk_mq_debugfs_unregister_hctxs(q);
> >                 blk_mq_sysfs_unregister_hctxs(q);
> > 
> > so I don't understand why you argue here about ->elevator_lock use?
> > 
> I am not arguing using ->elevator_lock wrt removal of hctx sysfs attributes
> as you explained that sysfs/kernfs already provides the needed protection. 
> But please see below my explanation.
> 
> >>
> >> Consider this case: While blk_mq_update_nr_hw_queues removes hctx attributes,
> >> and simultaneously if nr_requests is also updating num of tags, would that not 
> >> cause any side effect?
> > 
> > Why is updating nr_requests related with removing hctx attributes?
> > 
> > Can you explain the side effect in details?
> Thread 1:
> writing-to-blk-mq-sysfs-attribute-nr_requests 
>   -> queue_requests_store ==> freezes queue and acquires ->elevator_lock 
>     -> blk_mq_update_nr_requests 
>       -> blk_mq_tag_update_depth
>         -> blk_mq_alloc_map_and_rqs
>           -> blk_mq_alloc_rq_map
>             -> blk_mq_init_tags ==> updates ->nr_tags and ->nr_reserved_tags 
> 
> Thread2:
> blk_mq_update_nr_hw_queues
>   -> __blk_mq_update_nr_hw_queues
>     -> blk_mq_realloc_tag_set_tags
>       -> __blk_mq_alloc_map_and_rqs
>         -> blk_mq_alloc_map_and_rqs
>           -> blk_mq_alloc_rq_map
>             -> blk_mq_init_tags ==> updates ->nr_tags and ->nr_reserved_tags 
> 
> Thread 3:
> reading-hctx-sysfs-attribute-nr_tags
>   -> blk_mq_hw_sysfs_show ==> acquires ->elevaor_lock
>     ->  blk_mq_hw_sysfs_nr_tags_show ==> access nr_tags 
> 
> Thread 4:
> reading-hctx-sysfs-attribute-nr_reserved_tags
>   -> blk_mq_hw_sysfs_show ==> acquires ->elevaor_lock
>     -> blk_mq_hw_sysfs_nr_reserved_tags_show ==> access nr_reserved_tags

`hctx->tags` is guaranteed to be live if above ->show() method, and the
elevator lock is actually not needed, which isn't supposed to protect
hctx->tags too.

> 
> As we can see above, ->nr_tags and ->nr_reserved_tags are also exported 
> to userspace using hctx sysfs attributes (nr_tags and nr_reserved_tags).
> 
> So my point was,
> #1 For alleviating race between nr_hw_queues and nr_requests update,
>    we need protection (probably using srcu lock) so that ->nr_tags 
>    and ->nr_reserved_tags are not updated simultaneously.
> 
> #2 How could we protect race between thread 3 and thread 2 above or 
>    race between thread 4 and thread 2 above?

blk_mq_update_nr_hw_queues() calls blk_mq_sysfs_unregister_hctxs() first,
then user can not see the above attributes before calling blk_mq_sysfs_register_hctxs().

So there isn't the race.

> 
> > 
> >> Maybe we also want to protect blk_mq_update_nr_requests
> >> with srcu read lock (set->update_nr_hwq_srcu) so that it couldn't run while  
> >> blk_mq_update_nr_hw_queues is in progress?
> > 
> > Yeah, agree, and it can be one new patch for covering race between
> > blk_mq_update_nr_requests and blk_mq_update_nr_hw_queues, the point is just
> > that nr_hw_queues is being changed, and not related with removing hctx
> > attributes, IMO.
> > 
> Please note that blk_mq_update_nr_requests also updates q->nr_requests,

blk_mq_update_nr_requests() uses nr_hw_queues, so there is race between
blk_mq_update_nr_requests() and blk_mq_update_nr_hw_queues().

> however looking at all code paths which updates this value is already
> protected with ->elevator_lock. So the only thing which worries me
> about updates of ->nr_tags and ->nr_reserved tags as shown above.

As I mentioned, there isn't such race.


Thanks,
Ming