Re: [PATCH v2 2/4] blk-cgroup: fix race between policy activation and blkg destruction

[yu kuai]

Hi,

在 2026/6/25 23:08, Nilay Shroff 写道:
> On 6/24/26 12:16 PM, Yu Kuai wrote:
>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index 7baccfb690fe..f7e788a7fe95 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -1563,10 +1563,12 @@ int blkcg_activate_policy(struct gendisk 
>> *disk, const struct blkcg_policy *pol)
>>       if (WARN_ON_ONCE(!pol->pd_alloc_fn || !pol->pd_free_fn))
>>           return -EINVAL;
>>         if (queue_is_mq(q))
>>           memflags = blk_mq_freeze_queue(q);
>> +
>> +    mutex_lock(&q->blkcg_mutex);
>>   retry:
>>       spin_lock_irq(&q->queue_lock);
>>         /* blkg_list is pushed at the head, reverse walk to 
>> initialize parents first */
>>       list_for_each_entry_reverse(blkg, &q->blkg_list, q_node) {
>> @@ -1625,10 +1627,11 @@ int blkcg_activate_policy(struct gendisk 
>> *disk, const struct blkcg_policy *pol)
>>       __set_bit(pol->plid, q->blkcg_pols);
>>       ret = 0;
>>         spin_unlock_irq(&q->queue_lock);
>>   out:
>> +    mutex_unlock(&q->blkcg_mutex);
>>       if (queue_is_mq(q))
>>           blk_mq_unfreeze_queue(q, memflags);
>>       if (pinned_blkg)
>>           blkg_put(pinned_blkg);
>>       if (pd_prealloc)
>
> If the policy allocation fails, we jump to the lable enomem: and 
> teardown pds.
> But I see this path still only acquires ->queue_lock. Don't we also need
> to protect it with ->blkcg_mutex?

Yes, I agree we should protect it as well.

>
> Moreover I still see race between blkg insertion in blkg_create() which
> still doesn't use ->blkcg_mutex and so list traversal in 
> bfq_end_wr_async()
> may still race with blkg_create(), isn't it? I remember you once told
> this will be handled in another series but I couldn't find that yet.

This is the set:

[PATCH 0/8] blk-cgroup: remove queue_lock nesting from blkcg paths - Yu 
Kuai <https://lore.kernel.org/all/cover.1780621988.git.yukuai@xxxxxxx/>

Noted that set just make sure queue_lock is not nested under other atomic
context, and that set do not acquire blkcg_mutex for blkg_create() yet. Howerver,
with that set it'll be easy to convert all queue_lock to blkcg_mtuex for blkg
protection, and together with lots of blk-cgroup code cleanups.


>
> Thanks,
> --Nilay
>
>
>
>
-- 
Thanks,
Kuai