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] [day] [month] [year] [list]

Message-ID: <nqlzjsh267pnjh3gviw4bwtvviwiq5oso4pksngjcd46c5pmf3@l5cjx5h7akab>
Date: Mon, 28 Jul 2025 23:45:37 +0200
From: Jan Kara <jack@...e.cz>
To: Yu Kuai <yukuai@...nel.org>
Cc: Jan Kara <jack@...e.cz>, 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()

On Tue 29-07-25 01:08:40, Yu Kuai wrote:
> 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:

Just one spelling correction. Otherwise looks great to me.

> 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
							    ^^^^ thus the
found icq

> +        * is guaranteed to be valid until the io is done.
>          */
>         rcu_read_lock();
>         icq = rcu_dereference(ioc->icq_hint);

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR