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

[Tao Cui]

Hi Kuai,Christoph,
在 2026/8/4 23:30, yu kuai 写道:
> Hi,
> 
> 在 2026/8/4 21:32, Christoph Hellwig 写道:
>> On Tue, Aug 04, 2026 at 05:19:24PM +0800, Tao Cui wrote:
>>> While reading 2/3, one spot in bio_pinned_blkg() made me wonder, so I
>>> gave it a try — and the WARN_ON_ONCE triggers every time for me.
>>>
>>> I may well be missing something, but my worry is that the bio's ref on
>>> the blkg keeps the object alive, not its entry in the radix tree.
>>> blkg_destroy() runs throtl_pd_offline (which only schedules an async
>>> flush) before radix_tree_delete(), so the queued bio ends up dispatched
>>> (blk_throtl_dispatch_work_fn -> blk_cgroup_bio_start ->
>>> bio_pinned_blkg) after the blkg is already gone from the tree, and
>>> blkg_lookup() returns NULL.
>>>
>>> I applied the series and wrote a small reproducer:
>>>
>>>    - null_blk, cgroup v2, a child cgroup with io.max rbps=4096;
>>>    - a read issued in the child cgroup gets throttled and queued, pinning
>>>      the blkg;
>>>    - migrate the reader out and rmdir the cgroup; the queued bio is then
>>>      flushed after the blkg has left the tree.
>> Can you add this to blktests?
>>

yes, I'll turn the reproducer into a blktests case and send
it out. 

>>> Maybe keeping the pinned blkg pointer in the bio would sidestep this, so
>>> the lookup can't miss?
> 
> The problem here is that blkg_destroy can be called while blkg is still pinned
> by blkg_get, in this case remove the cgroup directly remove the blkg from radix
> tree, that's why blkg_lookup can't find this blkg anymore, and the extra blkg ref
> is leaked :(
> 
>> That would grow the bio, which we try hard to avoid.  I think the way to
>> avoid this is to have active/passive refcounts on the blkg, where an
>> active one keeps it in the radix tree, but a 0 passive one would prevent
>> the caller from getting a new reference to it.  The users who rely on the
>> pin for the I/O completion path would then just keep the active reference
>> and use a pure lookup without getting a new passive reference in the
>> completion path.  This would remove the need for BIO_BLKG_REF which
>> feels a bit kludgy and eats up precious bio flag space.
> 
> The problem here is that remove a cgroup can also remove the blkg from radix tree,
> even through it still has active refcounts. I think this can be fixed by checking
> the cgroup online_pin first, if it's zero, we can search the blkg from the
> request_queue blkg list, where blkg will not be removed until blkg_free_workfn().
> What's better, if we can convert the blkg list to hash table with key as blk-cgroup,
> it will be much better as we can lookup from this table instead of blkcg radix tree.
> 

Both of your approaches go further than my store-the-pointer idea;
looking forward to the next version.

Thanks,
Tao

>>