Hi Kuai. On Tue, Aug 04, 2026 at 02:53:12PM +0800, Yu Kuai <[email protected]> wrote: > From: Yu Kuai <[email protected]> > > 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
signature.asc
Description: PGP signature
