linux-kernel - Re: [PATCH v2] blk-ioc: don't hold queue_lock for ioc_lookup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9dc51c34-1e5d-4da2-bc00-a89e7c173073@kernel.org>
Date: Tue, 29 Jul 2025 01:08:40 +0800
From: Yu Kuai <yukuai@...nel.org>
To: Jan Kara <jack@...e.cz>
Cc: dlemoal@...nel.org, axboe@...nel.dk, linux-block@...r.kernel.org,
 linux-kernel@...r.kernel.org, yukuai3@...wei.com, yi.zhang@...wei.com,
 yangerkun@...wei.com, johnny.chenyi@...wei.com
Subject: Re: [PATCH v2] blk-ioc: don't hold queue_lock for ioc_lookup_icq()

Hi,

在 2025/7/28 17:28, Jan Kara 写道:
> On Sat 26-07-25 02:03:34, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@...wei.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@...wei.com>
> Looks good! Just one small comment below. With that fixed feel free to add:
>
> Reviewed-by: Jan Kara <jack@...e.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