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/[email protected]/>
>>
>> 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
