Re: [PATCH v2 3/3] blk-cgroup: move async bio punt state to blkcg
From: Nilay Shroff <hidden>
Date: 2026-09-13 13:31:10
Also in:
cgroups, dm-devel, gfs2, linux-bcache, linux-block, linux-doc, linux-fsdevel, linux-mm, lkml, llvm
On 9/13/26 12:24 PM, Yu Kuai wrote:
quoted hunk ↗ jump to hunk
From: Yu Kuai <yukuai@fygo.io> blkcg_punt_bio_submit() currently queues punted bios on blkg->async_bios, so it has to call bio_blkg() to find or create a queue-local blkg. Bios now carry and pin the blkcg css, so punted bio lifetime no longer needs to be anchored by a blkg. Keeping the punt state in blkg can instantiate a blkg even when no blkcg policy is enabled, just to bounce submission from a shared kthread. Move async_bio_lock, async_bios and async_bio_work to struct blkcg, and queue punted bios on bio_blkcg() for non-root cgroups. Root or unassociated bios are submitted directly. This preserves the priority-inversion avoidance while preventing blkcg_punt_bio_submit() from creating blkgs that are not needed by any policy. Signed-off-by: Yu Kuai <yukuai@fygo.io> --- block/blk-cgroup.c | 52 ++++++++++++++++++++++++++-------------------- block/blk-cgroup.h | 14 ++++++------- 2 files changed, 35 insertions(+), 31 deletions(-)diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index 59ccfefe16a8..aa3cee107ebe 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c@@ -180,14 +180,10 @@ static void blkg_free(struct blkcg_gq *blkg) static void __blkg_release(struct rcu_head *rcu) { struct blkcg_gq *blkg = container_of(rcu, struct blkcg_gq, rcu_head); -#ifdef CONFIG_BLK_CGROUP_PUNT_BIO - WARN_ON(!bio_list_empty(&blkg->async_bios)); -#endif - blkg_free(blkg); } /* * A group is RCU protected, but having an rcu lock does not mean that one@@ -226,23 +222,23 @@ static void blkg_release(struct percpu_ref *ref) } #ifdef CONFIG_BLK_CGROUP_PUNT_BIO static struct workqueue_struct *blkcg_punt_bio_wq; -static void blkg_async_bio_workfn(struct work_struct *work) +static void blkcg_async_bio_workfn(struct work_struct *work) { - struct blkcg_gq *blkg = container_of(work, struct blkcg_gq, - async_bio_work); + struct blkcg *blkcg = container_of(work, struct blkcg, async_bio_work); struct bio_list bios = BIO_EMPTY_LIST; struct bio *bio; struct blk_plug plug; bool need_plug = false; - /* as long as there are pending bios, @blkg can't go away */ - spin_lock(&blkg->async_bio_lock); - bio_list_merge_init(&bios, &blkg->async_bios); - spin_unlock(&blkg->async_bio_lock); + /* as long as there are pending bios, @blkcg can't go away */ + { + guard(spinlock)(&blkcg->async_bio_lock); + bio_list_merge_init(&bios, &blkcg->async_bios); + }
Instead of using guard(spinlock)(...) here, I think we could use the simpler spin_lock()/spin_unlock() helpers. IMO, they are easier to read and reason about for these short critical sections.
quoted hunk ↗ jump to hunk
/* start plug only when bio_list contains at least 2 bios */ if (bios.head && bios.head->bi_next) { need_plug = true; blk_start_plug(&plug);@@ -259,19 +255,20 @@ static void blkg_async_bio_workfn(struct work_struct *work) * cgroup. Use this helper instead of submit_bio to punt the actual issuing to * a dedicated per-blkcg work item to avoid such priority inversions. */ void blkcg_punt_bio_submit(struct bio *bio) { - struct blkcg_gq *blkg = bio_blkg(bio); + struct blkcg *blkcg = bio_blkcg(bio); - if (blkg && blkg->parent) { - spin_lock(&blkg->async_bio_lock); - bio_list_add(&blkg->async_bios, bio); - spin_unlock(&blkg->async_bio_lock); - queue_work(blkcg_punt_bio_wq, &blkg->async_bio_work); + if (blkcg && cgroup_parent(blkcg->css.cgroup)) { + { + guard(spinlock)(&blkcg->async_bio_lock); + bio_list_add(&blkcg->async_bios, bio); + } + queue_work(blkcg_punt_bio_wq, &blkcg->async_bio_work);
Again same here, replace guard() with spin_lock() and spin_unlock() helpers.
quoted hunk ↗ jump to hunk
} else { - /* Never bounce if there is no non-root blkg to queue on. */ + /* Never bounce if there is no non-root blkcg to queue on. */ submit_bio(bio); } } EXPORT_SYMBOL_GPL(blkcg_punt_bio_submit);@@ -350,15 +347,10 @@ static struct blkcg_gq *blkg_alloc(struct blkcg *blkcg, struct gendisk *disk, blkg->q = disk->queue; INIT_LIST_HEAD(&blkg->q_node); blkg->blkcg = blkcg; blkg->blkcg_id = blkcg->css.id; blkg->iostat.blkg = blkg; -#ifdef CONFIG_BLK_CGROUP_PUNT_BIO - spin_lock_init(&blkg->async_bio_lock); - bio_list_init(&blkg->async_bios); - INIT_WORK(&blkg->async_bio_work, blkg_async_bio_workfn); -#endif u64_stats_init(&blkg->iostat.sync); for_each_possible_cpu(cpu) { u64_stats_init(&per_cpu_ptr(blkg->iostat_cpu, cpu)->sync); per_cpu_ptr(blkg->iostat_cpu, cpu)->blkg = blkg;@@ -1399,10 +1391,16 @@ static void blkcg_css_free(struct cgroup_subsys_state *css) if (blkcg->cpd[i]) blkcg_policy[i]->cpd_free_fn(blkcg->cpd[i]); mutex_unlock(&blkcg_pol_mutex); +#ifdef CONFIG_BLK_CGROUP_PUNT_BIO + { + guard(spinlock)(&blkcg->async_bio_lock); + WARN_ON(!bio_list_empty(&blkcg->async_bios)); + } +#endif
This is a slightly different case. At this point blkcg_css_free() is
freeing the blkcg object after its final reference has gone away, so
there should be no concurrent context accessing blkcg->async_bios.
Therefore, I don't think we need to acquire async_bio_lock here just
to perform the WARN_ON() check.
The clang context annotation cannot infer this object-lifetime property
and will therefore report an unprotected access. I think we should
explicitly mark this access as context-unsafe.
But wait, even better, we could introduce a bio_list_empty_careful() helper,
similar to list_empty_careful(), for this purpose:
static inline bool bio_list_empty_careful(const struct bio_list *bl)
__context_unsafe(/* intentional lockless access to @bl->head */)
{
return bl->head == NULL;
}
Then this could simply become:
WARN_ON(!bio_list_empty_careful(&blkcg->async_bios));
quoted hunk ↗ jump to hunk
free_percpu(blkcg->lhead); kfree(blkcg); } static struct cgroup_subsys_state *@@ -1447,10 +1445,18 @@ blkcg_css_alloc(struct cgroup_subsys_state *parent_css) } spin_lock_init(&blkcg->lock); refcount_set(&blkcg->online_pin, 1); INIT_HLIST_HEAD(&blkcg->blkg_list); +#ifdef CONFIG_BLK_CGROUP_PUNT_BIO + spin_lock_init(&blkcg->async_bio_lock); + { + guard(spinlock)(&blkcg->async_bio_lock); + bio_list_init(&blkcg->async_bios); + } + INIT_WORK(&blkcg->async_bio_work, blkcg_async_bio_workfn); +#endif
This is interesting. As you know, while an object is being allocated and before it is published, it can't be accessed concurrently. So guarding blkcg->async_bios with blkcg->async_bio_lock is not necessary here. Moreover, since blkcg is zero-initialized, we could simply remove both the guard(...) and the bio_list_init() call above. Thanks, --Nilay