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

Hi,

=E5=9C=A8 2026/8/4 21:32, Christoph Hellwig =E5=86=99=E9=81=93:
> 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 =E2=80=94 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=3D4096;
>>    - a read issued in the child cgroup gets throttled and queued, pinnin=
g
>>      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?
>
>> 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 pin=
ned
by blkg_get, in this case remove the cgroup directly remove the blkg from r=
adix
tree, that's why blkg_lookup can't find this blkg anymore, and the extra bl=
kg 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 radi=
x tree,
even through it still has active refcounts. I think this can be fixed by ch=
ecking
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_wor=
kfn().
What's better, if we can convert the blkg list to hash table with key as bl=
k-cgroup,
it will be much better as we can lookup from this table instead of blkcg ra=
dix tree.

>
--=20
Thanks,
Kuai