Re: [RFC PATCH v1 1/3] blk-cgroup: add helpers for bio cgroup state

Re: [RFC PATCH v1 1/3] blk-cgroup: add helpers for bio cgroup state

[Christoph Hellwig]

On Tue, Aug 04, 2026 at 12:52:16PM +0200, Jan Kara wrote:
> Mostly looks good. Just I think bio_blkcg() should gracefully handle the
> case where bio->bi_blkg is NULL (and return NULL in that case). That way
> you can also get rid of somewhat odd pattern:
> 
> 	if (!bio_blkg(bio))
> 		return ...;
> 	do something with bio_blkcg(bio)
> 
> You can then just check bio_blkcg(bio) directly which is much more obvious.

Yes.  Looking at the whole series I'm also not sure that this makes too
much sense as a split out patch as the next one touches more than half
of the callsite anyway.

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

[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?

> Maybe keeping the pinned blkg pointer in the bio would sidestep this, so
> the lookup can't miss?

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.

Re: [RFC PATCH v1 1/3] blk-cgroup: add helpers for bio cgroup state

[yu kuai]

Hi,

在 2026/8/4 21:25, Christoph Hellwig 写道:
> On Tue, Aug 04, 2026 at 12:52:16PM +0200, Jan Kara wrote:
>> Mostly looks good. Just I think bio_blkcg() should gracefully handle the
>> case where bio->bi_blkg is NULL (and return NULL in that case). That way
>> you can also get rid of somewhat odd pattern:
>>
>> 	if (!bio_blkg(bio))
>> 		return ...;
>> 	do something with bio_blkcg(bio)
>>
>> You can then just check bio_blkcg(bio) directly which is much more obvious.
> Yes.  Looking at the whole series I'm also not sure that this makes too
> much sense as a split out patch as the next one touches more than half
> of the callsite anyway.

Yes, this make sense, I'm trying not to cook a huge patch, but patch 2 is grow
much bigger than I was expected. Unless I figure out a nicer way to split patch 2,
I'll merge them in the next version.

>
-- 
Thanks,
Kuai

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

[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?
>
>> 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.

>
-- 
Thanks,
Kuai

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

[Christoph Hellwig]

On Tue, Aug 04, 2026 at 11:30:55PM +0800, yu kuai wrote:
> 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.

Yes, I think you can really easily do that with the rhashtable.

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

>>

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

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 3825 bytes --]

Hi Kuai.

On Tue, Aug 04, 2026 at 02:53:12PM +0800, Yu Kuai <yukuai@kernel.org> wrote:
> From: Yu Kuai <yukuai@fygo.io>
> 
> A bio currently stores a queue-local blkg reference.  This forces bio
> association and remap paths to look up or create a blkg even when the bio
> will never enter a blkcg policy.
> 
> Store the blkcg css association in the bio instead, and derive the blkg
> from the bio's blkcg and current bdev when a policy needs it.  The first
> successful policy lookup pins the blkg, records the pin with BIO_BLKG_REF,
> and drops it from bio_clear_blkcg() or when bio_set_dev() changes the
> lookup key.
> 
> Keep lookup-only users from creating missing blkgs by using
> bio_blkg_lookup(), and rename the bio cgroup association helpers to match
> the stored blkcg state.

I assume this should be OK due to limited lifetime of bios -- this would
not lead no possibly indefinite accumulation of offlined blkcgs,
correct?

> -void bio_associate_blkg_from_css(struct bio *bio,
> +void bio_associate_blkcg_from_css(struct bio *bio,
>  				 struct cgroup_subsys_state *css)
>  {
> -	if (bio_blkg(bio))
> -		blkg_put(bio_blkg(bio));
> +	struct blkcg *blkcg;
>  
> -	if (css && css->parent) {
> -		bio->bi_blkg = blkg_tryget_closest(bio, css);
> -	} else {
> -		blkg_get(bdev_get_queue(bio->bi_bdev)->root_blkg);
> -		bio->bi_blkg = bdev_get_queue(bio->bi_bdev)->root_blkg;
> -	}
> +	if (!css || !css->parent)
> +		css = &blkcg_root.css;
> +
> +	blkcg = css_to_blkcg(css);
> +	if (bio_blkcg(bio) == blkcg)
> +		return;
> +
> +	css_get(css);	<---
> +	bio_clear_blkcg(bio);
> +	bio->bi_blkcg = blkcg;
>  }
> -EXPORT_SYMBOL_GPL(bio_associate_blkg_from_css);
> +EXPORT_SYMBOL_GPL(bio_associate_blkcg_from_css);

[skip to next comment below now]

And here yet another (any) reference to same css is taken 2nd time.

[skip after next comment below :)]


>  
>  /**
> - * bio_associate_blkg - associate a bio with a blkg
> + * bio_associate_blkcg - associate a bio with a blkcg
>   * @bio: target bio
>   *
> - * Associate @bio with the blkg found from the bio's css and request_queue.
> - * If one is not found, bio_lookup_blkg() creates the blkg.  If a blkg is
> - * already associated, the css is reused and association redone as the
> - * request_queue may have changed.
> + * Associate @bio with the blkcg found from the bio's css.  If a blkcg is
> + * already associated, keep it as blkcg association is not queue-local.
>   */
> -void bio_associate_blkg(struct bio *bio)


> +void bio_associate_blkcg(struct bio *bio)
>  {
>  	struct cgroup_subsys_state *css;
>  
>  	if (blk_op_is_passthrough(bio->bi_opf))
>  		return;
>  
> -	if (bio_blkg(bio)) {
> -		css = bio_blkcg_css(bio);
> -		bio_associate_blkg_from_css(bio, css);
> -	} else {
> -		rcu_read_lock();
> -		css = blkcg_css();
> -		if (!css_tryget_online(css))
> -			css = NULL;
> -		rcu_read_unlock();
> +	if (bio_blkcg(bio))
> +		return;
>  
> -		bio_associate_blkg_from_css(bio, css);
> -		if (css)
> -			css_put(css);
> -	}
> +	rcu_read_lock();
> +	css = blkcg_css();
> +	if (!css_tryget_online(css))	<---
> +		css = NULL;
> +	rcu_read_unlock();
> +
> +	bio_associate_blkcg_from_css(bio, css);
> +	if (css)
> +		css_put(css);
>  }
> -EXPORT_SYMBOL_GPL(bio_associate_blkg);
> +EXPORT_SYMBOL_GPL(bio_associate_blkcg);

next:
Here you take (online) reference to the blkcg->css.

[return back to previous comment]


after:
Ideally, no tasks should be in offlined (blk)cgs, so the `current` would
not resolve to blkcg_css() returning an offlined blkcgs. OTOH, it's
generally good not to do _new_ associations to an offlined blkcg.
Which is why I think this logic would better fit to
bio_associate_blkcg_from_css()


0.02€,
Michal


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]