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

[Nilay Shroff <nilay@linux.ibm.com>]

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.

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? 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?

Thanks,
--Nilay