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
