Re: [PATCH v2] blk-ioc: don't hold queue_lock for ioc_lookup_icq()

[Yu Kuai <yukuai@kernel.org>]

Hi,

在 2025/7/28 17:28, Jan Kara 写道:
> On Sat 26-07-25 02:03:34, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Currently issue io can grab queue_lock three times from bfq_bio_merge(),
>> bfq_limit_depth() and bfq_prepare_request(), the queue_lock is not
>> necessary if icq is already created because both queue and ioc can't be
>> freed before io issuing is done, hence remove the unnecessary queue_lock
>> and use rcu to protect radix tree lookup.
>>
>> Noted this is also a prep patch to support request batch dispatching[1].
>>
>> [1] https://lore.kernel.org/all/20250722072431.610354-1-yukuai1@huaweicloud.com/
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> Looks good! Just one small comment below. With that fixed feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
>> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
>> index ce82770c72ab..ea9c975aaef7 100644
>> --- a/block/blk-ioc.c
>> +++ b/block/blk-ioc.c
>> @@ -308,19 +308,18 @@ int __copy_io(unsigned long clone_flags, struct task_struct *tsk)
>>   
>>   #ifdef CONFIG_BLK_ICQ
>>   /**
>> - * ioc_lookup_icq - lookup io_cq from ioc
>> + * ioc_lookup_icq - lookup io_cq from ioc in io issue path
>>    * @q: the associated request_queue
>>    *
>>    * Look up io_cq associated with @ioc - @q pair from @ioc.  Must be called
>> - * with @q->queue_lock held.
>> + * from io issue path, either return NULL if current issue io to @q for the
>> + * first time, or return a valid icq.
>>    */
>>   struct io_cq *ioc_lookup_icq(struct request_queue *q)
>>   {
>>   	struct io_context *ioc = current->io_context;
>>   	struct io_cq *icq;
>>   
>> -	lockdep_assert_held(&q->queue_lock);
>> -
>>   	/*
>>   	 * icq's are indexed from @ioc using radix tree and hint pointer,
>>   	 * both of which are protected with RCU.  All removals are done
> In this comment there's still reference to holding 'q lock'. I think you
> should replace that with justification why when called from IO issue path
> we are guaranteed found pointer is safe to use.
Thanks for the review! How about:

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index ea9c975aaef7..fd77e655544f 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -322,9 +322,9 @@ struct io_cq *ioc_lookup_icq(struct request_queue *q)

         /*
          * icq's are indexed from @ioc using radix tree and hint pointer,
-        * both of which are protected with RCU.  All removals are done
-        * holding both q and ioc locks, and we're holding q lock - if we
-        * find a icq which points to us, it's guaranteed to be valid.
+        * both of which are protected with RCU, io issue path ensures that
+        * both request_queue and current task are valid, the founded icq
+        * is guaranteed to be valid until the io is done.
          */
         rcu_read_lock();
         icq = rcu_dereference(ioc->icq_hint);

>
> 								Honza