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@xxxxxxxxxx>
>>>>> ---
>>>>> 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@xxxxxxxxxxxxx/
>>>
>> 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
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?
>
>> 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,
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.
Thanks,
--Nilay
