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

[yu kuai]

Hi,

在 2026/9/7 14:02, 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.

Turns out Clang will still warn after adding rcu_read_lock(). I'm not familiar how this
works, but AI told me this is related to inline helpers, And I'll have to add the same
__must_hold_shared(RCU) in rhashtable_lookup() for following inline callers to kill the
warning:

   static inline struct blkcg_gq *blkg_lookup_any(...)
         __must_hold_shared(RCU);

   static inline struct blkcg_gq *blkg_lookup(...)
         __must_hold_shared(RCU);

>
> Thanks,
> --Nilay

-- 
Thanks,
Kuai