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 05:51:07PM +0530, Nilay Shroff wrote:
> 
> 
> On 4/15/25 5:24 PM, Ming Lei wrote:
> >>>
> >>> 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.
> > 
> I think, the ->elavtor_lock would still be needed for protecting updates to 
> hctx->tags from thread # 1 above and simultaneously reading the hctx->tags from 
> thread #3 and #4 above.

update_nr_requests() can just reduce ->tags.sbitmap size, please see
blk_mq_tag_update_depth(), even it won't change hctx->tags->nr_tags, so there
isn't race.

More importantly ->elevator_lock shouldn't cover ->tags which isn't related
with scheduler, one exception is blk-mq debugfs, in which request may be
allocated from ->sched_tags, even ->tags is visiting.


Thanks, 
Ming