Re: [PATCH 2/3] blk-cgroup: store blkcg in bio instead of blkg

[Nilay Shroff]

On 9/6/26 5:09 PM, yu kuai wrote:
> Hi,
>
> 在 2026/9/6 18:56, Nilay Shroff 写道:
> > On 8/23/26 7:00 PM, Yu Kuai wrote:
> > > @@ -257,20 +262,21 @@ static inline bool
> > > bio_issue_as_root_blkg(struct bio *bio)
> > >     * @blkcg: blkcg of interest
> > >     * @q: request_queue of interest
> > >     *
> > >     * Lookup a blkg for the @blkcg - @q pair, whether it is online or
> > > dying.
> > >     *
> > > - * Must be called in a RCU critical section.
> > > + * Must be called with either RCU read lock or queue_lock held.
> > >     *
> > >     * This does not acquire a reference.  The caller must already hold
> > > one, or
> > >     * have the blkg pinned by I/O.
> > >     */
> > >    static inline struct blkcg_gq *blkg_lookup_any(struct blkcg *blkcg,
> > >                               struct request_queue *q)
> > >    {
> > > -    RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
> > > -             "blkg_lookup_any() requires an RCU read lock");
> > > +    RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&
> > > +             !lockdep_is_held(&q->queue_lock),
> > > +             "blkg_lookup_any() requires an RCU read lock or
> > > queue_lock");
> > >          return rhashtable_lookup(&q->blkg_hash, &blkcg->css.id,
> > >                     blkg_hash_params);
> > >    }
> >
> > The changes look good to me. However, rhashtable_lookup() is annotated
> > with __must_hold_shared(RCU). Since we now officially support Clang
> > context annotations in the block layer, compiling with the latest
> > Clang/LLVM generates a number of context warnings, for example:
> >
> > block/blk-cgroup.h:277:9: warning: calling function 'rhashtable_lookup'
> > requires holding __ctx_lock_RCU 'RCU' [-Wthread-safety-analysis]
> >    277 |         return rhashtable_lookup(&q->blkg_hash, &blkcg->css.id,
> >        |                ^
> >
> > The same warning is reported from several other block layer source
> > files that include blk-cgroup.h.
> >
> > This indicates that rhashtable_lookup() requires the RCU read lock
> > from Clang's context-analysis perspective. Looking at the call paths of
> > blkg_lookup_any(), it appears that not all callers hold
> > rcu_read_lock() when invoking it.
>
> I was not aware of the above clang warning, this should not be a real problem
> because queue_lock should have the same protection as rcu, and I was trying
> not to make this patch too complex.
I agree that this is not a runtime correctness issue when the lookup is already
protected by q->queue_lock.

> I add the rcu_read_lock() in the following patch to covert queue_lock to blkcg_mutex:
>
> [RFC PATCH v3 3/6] blk-cgroup: protect blkgs with blkcg_mutex - Yu Kuai <https://lore.kernel.org/all/20260823152926.1043863-4-yukuai@xxxxxxxxxx/>
>
> Since this is related to compile warning, I'll move the rcu protection to this
> patch.
Since rhashtable_lookup() is annotated with __must_hold_shared(RCU), Clang's
context analysis still reports warnings for these call paths. While this
doesn't cause a build failure (and only generates warning) when CONFIG_WERROR
is not enabled, it could do so for configurations that enable CONFIG_WERROR=y.

So IMO, moving the rcu_read_lock() protection into this patch makes sense.
This would avoid introducing the Clang warnings/errors in this patch, and then
you may accordingly adjust your subsequent series where the q->queue_lock protection
is removed and the blkg lookup paths are protected by RCU.

I also noticed that some paths already take q->queue_lock followed by rcu_read_lock(),
so adding the RCU protection here should be consistent with those existing paths.

Thanks,
--Nilay