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

[Nilay Shroff <nilay@linux.ibm.com>]

On 9/12/26 7:51 PM, yu kuai wrote:
> 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@kernel.org/>
>>>
>>> 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);
> 
Yes, that's correct. In this case the context requirement needs to be propagated through
the inline helper call chain.

Thanks,
--Nilay