[RFC PATCH v3 0/6] blk-cgroup: protect blkgs with blkcg_mutex

[RFC PATCH v3 0/6] blk-cgroup: protect blkgs with blkcg_mutex

[Yu Kuai]

From: Yu Kuai <yukuai@fygo.io>

This RFC moves queue-local blkg topology synchronization from
q->queue_lock to q->blkcg_mutex.  It is based on the preparatory series
which stores a queue-independent blkcg in each bio and uses a
request_queue-owned rhashtable for blkg lookup.

q->queue_lock also protects block core state and is taken from atomic
contexts.  Keeping blkg creation, destruction, configuration and policy
lifecycle operations under that lock prevents those paths from using
sleepable operations and couples blkcg policy state to unrelated queue
locking.

Before switching locks, patch 1 moves pd_free_fn() calls outside spinlocks;
iocost's free callback can wait for an hrtimer callback on PREEMPT_RT.
Patch 2 gives throttle runtime state a private spinlock and shuts down its
timers before freeing their state.  Patch 3 then moves blkg topology and
policy synchronization to blkcg_mutex while retaining an RCU fast path for
existing-blkg I/O.  Patches 4 and 5 move allocation into blkg_create() and
share hierarchy creation with the configuration path.  Patch 6 keeps blkg
creation lazy for policy users and handles REQ_NOWAIT without sleeping,
falling back to the closest existing blkg when creation cannot proceed.

A git branch is available at:
  https://git.kernel.org/pub/scm/linux/kernel/git/yukuai/linux.git/log/?h=block-7.3-blkcg_mutex-v3

Changes since RFC v2:

  - Base the series on the bio/blkcg preparatory series instead of moving
    blkg association into submit_bio().  Drop old patch 1 accordingly.
  - Drop old patch 4 because the preparatory series replaces the per-blkcg
    radix tree with a request_queue rhashtable, so radix-tree preloading no
    longer exists.
  - Drop old patch 7 because bios now retain their blkcg independently and
    BFQ can use the blkg already pinned by the bio.
  - Add patch 1 to detach policy data under the existing spinlocks but call
    pd_free_fn() after dropping them.  Keep this fix separate so the mutex
    conversion does not redesign generic policy teardown.
  - In patch 2, shut down all per-group and top-level throttle timers
    before freeing throtl_data.  Use timer_shutdown_sync() for final
    teardown so a concurrent callback cannot rearm a timer after shutdown.
  - In patch 3, keep bio_blkg() on an RCU fast path when the target blkg
    already exists, taking blkcg_mutex only when hierarchy creation is
    needed.
  - In patch 3, hold blkcg_mutex for the complete blkg_destroy_all() walk
    and remove the spinlock-era batch counter and cond_resched() restart.
  - Adapt patches 4 and 5 to the rhashtable-based lookup and make
    blkg_lookup_create() return the closest existing blkg through an out
    parameter without acquiring a reference.
  - Rewrite old patch 8 as patch 6.  Keep allocation lazy in bio_blkg()
    instead of preparing every REQ_NOWAIT bio in submit_bio_noacct().  Use
    mutex_trylock() and GFP_ATOMIC when creation is possible; otherwise
    lookup and pin the closest existing blkg under RCU.  Valid policy I/O
    therefore does not fail with BLK_STS_AGAIN or receive a NULL blkg.
  - Do not add nowait-specific changes to blk-throttle, blk-iocost or
    blk-iolatency because bio_blkg() retains its non-NULL result for valid
    policy I/O.

Changes since RFC v1:

  - Rework the series on top of "associate blkg in submit_bio instead of
    bio_set_dev".
  - Drop the per-subsystem bio_set_dev() workarounds: NVMe multipath
    retarget (v1 patch 1), dm-thin (v1 patch 2), dm-snapshot (v1 patch 3),
    bcache (v1 patch 8), dm-bufio (v1 patch 9), dm-pcache (v1 patch 10) and
    DM NOWAIT remaps (v1 patch 12).  They are no longer needed because
    bio_set_dev() no longer associates a blkg.
  - Drop atomic bio allocation: bio_alloc_atomic() (v1 patch 5) and
    non-blocking bio allocation with a bdev (v1 patch 7).  The
    nd_virtio/ocfs2 callers are handled separately.
  - Drop the nowait-bio-allocation association helpers (v1 patches 6 and
    11).  Nowait is handled once at submission by failing the bio.
  - Keep and adapt the blk-throttle private runtime lock (v1 patch 4), the
    blkcg_mutex conversion (v1 patch 14), radix preload removal (v1 patch
    15), blkg_create() allocation (v1 patch 16), shared creation (v1 patch
    17) and the BFQ locked-cgroup-update fix (v1 patch 13).

Previous versions:
  RFC v2:
    https://lore.kernel.org/r/20260724123037.3004560-1-yukuai@kernel.org
  RFC v1:
    https://lore.kernel.org/r/20260704195124.1375075-1-yukuai@kernel.org

Yu Kuai (6):
  blk-cgroup: call pd_free_fn() outside spinlocks
  blk-throttle: protect throttle state with td lock
  blk-cgroup: protect blkgs with blkcg_mutex
  blk-cgroup: allocate blkgs in blkg_create
  blk-cgroup: share blkg creation between lookup and config prep
  blk-cgroup: make policy blkg creation nowait-safe

 block/bfq-cgroup.c    |  10 +-
 block/blk-cgroup.c    | 270 ++++++++++++++++++------------------------
 block/blk-cgroup.h    |  11 +-
 block/blk-iocost.c    |   8 +-
 block/blk-iolatency.c |   7 +-
 block/blk-throttle.c  |  93 +++++++++++----
 6 files changed, 207 insertions(+), 192 deletions(-)


base-commit: 81a5d989c04055860a66923e41151fd7faf4c295
-- 
2.51.0

[RFC PATCH v3 1/6] blk-cgroup: call pd_free_fn() outside spinlocks

[Yu Kuai]

From: Yu Kuai <yukuai@fygo.io>

blkcg_policy_teardown_pds() calls pd_free_fn() while holding both
q->queue_lock and blkcg->lock.  This is not safe for policies such as
iocost, whose ioc_pd_free() calls hrtimer_cancel().  On PREEMPT_RT the
hrtimer cancellation slow path can sleep while waiting for a soft hrtimer
callback to finish.

Keep the offline callback and policy data detachment protected by the
existing spinlocks, but tear down one policy data object at a time and drop
the locks before invoking pd_free_fn().  q->blkcg_mutex serializes the
operation against blkg_free_workfn(), so the associated blkg remains valid
while the callback runs.

Fixes: 7caa47151ab2 ("blkcg: implement blk-iocost")
Signed-off-by: Yu Kuai <yukuai@fygo.io>
---
 block/blk-cgroup.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 1bd91223367c..5b51be2fefc1 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1548,33 +1548,51 @@ struct cgroup_subsys io_cgrp_subsys = {
 	.depends_on = 1 << memory_cgrp_id,
 #endif
 };
 EXPORT_SYMBOL_GPL(io_cgrp_subsys);
 
-/*
- * Tear down per-blkg policy data for @pol on @q.
- */
-static void blkcg_policy_teardown_pds(struct request_queue *q,
-				      const struct blkcg_policy *pol)
+static struct blkg_policy_data *
+blkcg_policy_detach_pd(struct request_queue *q,
+		       const struct blkcg_policy *pol)
 {
+	struct blkg_policy_data *pd = NULL;
 	struct blkcg_gq *blkg;
 
+	lockdep_assert_held(&q->blkcg_mutex);
+
+	spin_lock_irq(&q->queue_lock);
 	list_for_each_entry(blkg, &q->blkg_list, q_node) {
 		struct blkcg *blkcg = blkg->blkcg;
-		struct blkg_policy_data *pd;
 
 		spin_lock(&blkcg->lock);
 		pd = blkg->pd[pol->plid];
 		if (pd) {
 			if (pd->online && pol->pd_offline_fn)
 				pol->pd_offline_fn(pd);
 			pd->online = false;
-			pol->pd_free_fn(pd);
 			WRITE_ONCE(blkg->pd[pol->plid], NULL);
 		}
 		spin_unlock(&blkcg->lock);
+
+		if (pd)
+			break;
 	}
+	spin_unlock_irq(&q->queue_lock);
+
+	return pd;
+}
+
+/*
+ * Tear down per-blkg policy data for @pol on @q.
+ */
+static void blkcg_policy_teardown_pds(struct request_queue *q,
+				      const struct blkcg_policy *pol)
+{
+	struct blkg_policy_data *pd;
+
+	while ((pd = blkcg_policy_detach_pd(q, pol)))
+		pol->pd_free_fn(pd);
 }
 
 /**
  * blkcg_activate_policy - activate a blkcg policy on a gendisk
  * @disk: gendisk of interest
@@ -1687,13 +1705,11 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
 		pol->pd_free_fn(pd_prealloc);
 	return ret;
 
 enomem:
 	/* alloc failed, take down everything */
-	spin_lock_irq(&q->queue_lock);
 	blkcg_policy_teardown_pds(q, pol);
-	spin_unlock_irq(&q->queue_lock);
 	ret = -ENOMEM;
 	goto out;
 }
 EXPORT_SYMBOL_GPL(blkcg_activate_policy);
 
@@ -1719,12 +1735,13 @@ void blkcg_deactivate_policy(struct gendisk *disk,
 
 	mutex_lock(&q->blkcg_mutex);
 	spin_lock_irq(&q->queue_lock);
 
 	__clear_bit(pol->plid, q->blkcg_pols);
-	blkcg_policy_teardown_pds(q, pol);
 	spin_unlock_irq(&q->queue_lock);
+
+	blkcg_policy_teardown_pds(q, pol);
 	mutex_unlock(&q->blkcg_mutex);
 
 	if (queue_is_mq(q))
 		blk_mq_unfreeze_queue(q, memflags);
 }
-- 
2.51.0

[RFC PATCH v3 2/6] blk-throttle: protect throttle state with td lock

[Yu Kuai]

From: Yu Kuai <yukuai@fygo.io>

Throttle currently uses queue_lock for both blkcg topology and its own
runtime state. This blocks moving blkg topology protection to blkcg_mutex
cleanly.

Add a throttle-private spinlock and use it for throttle service queues,
pending timers, runtime counters and config updates. Keep queue_lock only
where the current intermediate code still walks blkcg topology.

blkg_destroy_all() offlines policy data, but the data is freed
asynchronously. A per-group pending timer can therefore outlive
blk_throtl_exit(), which frees td before the policy data is released.
Previously, the timer callback took queue_lock and checked q->root_blkg
before dereferencing td, so a late per-group callback exited after
teardown. Once the callback takes td->lock, it dereferences td before that
check and can access freed memory.

Shut down all per-group and top-level timers before freeing td. Use
timer_shutdown_sync() instead of timer_delete_sync() because this is final
teardown: it waits for active callbacks and prevents any later mod_timer()
from rearming the timer. Use the same shutdown semantics before freeing a
throtl_grp.

Signed-off-by: Yu Kuai <yukuai@fygo.io>
---
 block/blk-throttle.c | 87 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 20 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 3828c3857900..2ff30700e84e 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -28,10 +28,13 @@ static struct workqueue_struct *kthrotld_workqueue;
 
 #define rb_entry_tg(node)	rb_entry((node), struct throtl_grp, rb_node)
 
 struct throtl_data
 {
+	/* protects throttle service queues and group runtime state */
+	spinlock_t lock;
+
 	/* service tree for active throtl groups */
 	struct throtl_service_queue service_queue;
 
 	struct request_queue *queue;
 
@@ -344,15 +347,20 @@ static void tg_update_has_rules(struct throtl_grp *tg)
 }
 
 static void throtl_pd_online(struct blkg_policy_data *pd)
 {
 	struct throtl_grp *tg = pd_to_tg(pd);
+	struct throtl_data *td = tg->td;
+	unsigned long flags;
+
+	spin_lock_irqsave(&td->lock, flags);
 	/*
 	 * We don't want new groups to escape the limits of its ancestors.
 	 * Update has_rules[] after a new group is brought online.
 	 */
 	tg_update_has_rules(tg);
+	spin_unlock_irqrestore(&td->lock, flags);
 }
 
 static void tg_release(struct rcu_head *rcu)
 {
 	struct blkg_policy_data *pd =
@@ -366,11 +374,11 @@ static void tg_release(struct rcu_head *rcu)
 
 static void throtl_pd_free(struct blkg_policy_data *pd)
 {
 	struct throtl_grp *tg = pd_to_tg(pd);
 
-	timer_delete_sync(&tg->service_queue.pending_timer);
+	timer_shutdown_sync(&tg->service_queue.pending_timer);
 	call_rcu(&pd->rcu_head, tg_release);
 }
 
 static struct throtl_grp *
 throtl_rb_first(struct throtl_service_queue *parent_sq)
@@ -1140,13 +1148,13 @@ static void throtl_pending_timer_fn(struct timer_list *t)
 	if (tg)
 		q = tg->pd.blkg->q;
 	else
 		q = td->queue;
 
-	spin_lock_irq(&q->queue_lock);
+	spin_lock_irq(&td->lock);
 
-	if (!q->root_blkg)
+	if (!READ_ONCE(q->root_blkg))
 		goto out_unlock;
 
 again:
 	parent_sq = sq->parent_sq;
 	dispatched = false;
@@ -1166,13 +1174,13 @@ static void throtl_pending_timer_fn(struct timer_list *t)
 
 		if (throtl_schedule_next_dispatch(sq, false))
 			break;
 
 		/* this dispatch windows is still open, relax and repeat */
-		spin_unlock_irq(&q->queue_lock);
+		spin_unlock_irq(&td->lock);
 		cpu_relax();
-		spin_lock_irq(&q->queue_lock);
+		spin_lock_irq(&td->lock);
 	}
 
 	if (!dispatched)
 		goto out_unlock;
 
@@ -1191,11 +1199,11 @@ static void throtl_pending_timer_fn(struct timer_list *t)
 	} else {
 		/* reached the top-level, queue issuing */
 		queue_work(kthrotld_workqueue, &td->dispatch_work);
 	}
 out_unlock:
-	spin_unlock_irq(&q->queue_lock);
+	spin_unlock_irq(&td->lock);
 }
 
 /**
  * blk_throtl_dispatch_work_fn - work function for throtl_data->dispatch_work
  * @work: work item being executed
@@ -1207,23 +1215,22 @@ static void throtl_pending_timer_fn(struct timer_list *t)
 static void blk_throtl_dispatch_work_fn(struct work_struct *work)
 {
 	struct throtl_data *td = container_of(work, struct throtl_data,
 					      dispatch_work);
 	struct throtl_service_queue *td_sq = &td->service_queue;
-	struct request_queue *q = td->queue;
 	struct bio_list bio_list_on_stack;
 	struct bio *bio;
 	struct blk_plug plug;
 	int rw;
 
 	bio_list_init(&bio_list_on_stack);
 
-	spin_lock_irq(&q->queue_lock);
+	spin_lock_irq(&td->lock);
 	for (rw = READ; rw <= WRITE; rw++)
 		while ((bio = throtl_pop_queued(td_sq, NULL, rw)))
 			bio_list_add(&bio_list_on_stack, bio);
-	spin_unlock_irq(&q->queue_lock);
+	spin_unlock_irq(&td->lock);
 
 	if (!bio_list_empty(&bio_list_on_stack)) {
 		blk_start_plug(&plug);
 		while ((bio = bio_list_pop(&bio_list_on_stack)))
 			submit_bio_noacct_nocheck(bio, false);
@@ -1297,11 +1304,11 @@ static void tg_conf_updated(struct throtl_grp *tg, bool global)
 			continue;
 	}
 	rcu_read_unlock();
 
 	/*
-	 * We're already holding queue_lock and know @tg is valid.  Let's
+	 * We're already holding td->lock and know @tg is valid.  Let's
 	 * apply the new config directly.
 	 *
 	 * Restart the slices for both READ and WRITES. It might happen
 	 * that a group's limit are dropped suddenly and we don't want to
 	 * account recently dispatched IO with new low rate.
@@ -1325,10 +1332,11 @@ static int blk_throtl_init(struct gendisk *disk)
 	td = kzalloc_node(sizeof(*td), GFP_KERNEL, q->node);
 	if (!td)
 		return -ENOMEM;
 
 	INIT_WORK(&td->dispatch_work, blk_throtl_dispatch_work_fn);
+	spin_lock_init(&td->lock);
 	throtl_service_queue_init(&td->service_queue);
 
 	memflags = blk_mq_freeze_queue(disk->queue);
 	blk_mq_quiesce_queue(disk->queue);
 
@@ -1379,18 +1387,20 @@ static ssize_t tg_set_conf(struct kernfs_open_file *of,
 		goto unprep;
 	if (!v)
 		v = U64_MAX;
 
 	tg = blkg_to_tg(ctx.blkg);
+	spin_lock_irq(&tg->td->lock);
 	tg_update_carryover(tg);
 
 	if (is_u64)
 		*(u64 *)((void *)tg + of_cft(of)->private) = v;
 	else
 		*(unsigned int *)((void *)tg + of_cft(of)->private) = v;
 
 	tg_conf_updated(tg, false);
+	spin_unlock_irq(&tg->td->lock);
 	ret = 0;
 
 unprep:
 	blkg_conf_unprep(&ctx);
 
@@ -1561,10 +1571,11 @@ static ssize_t tg_set_limit(struct kernfs_open_file *of,
 	ret = blkg_conf_prep(blkcg, &blkcg_policy_throtl, &ctx);
 	if (ret)
 		goto close_bdev;
 
 	tg = blkg_to_tg(ctx.blkg);
+	spin_lock_irq(&tg->td->lock);
 	tg_update_carryover(tg);
 
 	v[0] = tg->bps[READ];
 	v[1] = tg->bps[WRITE];
 	v[2] = tg->iops[READ];
@@ -1584,15 +1595,15 @@ static ssize_t tg_set_limit(struct kernfs_open_file *of,
 
 		ret = -EINVAL;
 		p = tok;
 		strsep(&p, "=");
 		if (!p || (sscanf(p, "%llu", &val) != 1 && strcmp(p, "max")))
-			goto unprep;
+			goto unlock;
 
 		ret = -ERANGE;
 		if (!val)
-			goto unprep;
+			goto unlock;
 
 		ret = -EINVAL;
 		if (!strcmp(tok, "rbps"))
 			v[0] = val;
 		else if (!strcmp(tok, "wbps"))
@@ -1600,20 +1611,24 @@ static ssize_t tg_set_limit(struct kernfs_open_file *of,
 		else if (!strcmp(tok, "riops"))
 			v[2] = min_t(u64, val, UINT_MAX);
 		else if (!strcmp(tok, "wiops"))
 			v[3] = min_t(u64, val, UINT_MAX);
 		else
-			goto unprep;
+			goto unlock;
 	}
 
 	tg->bps[READ] = v[0];
 	tg->bps[WRITE] = v[1];
 	tg->iops[READ] = v[2];
 	tg->iops[WRITE] = v[3];
 
 	tg_conf_updated(tg, false);
+	spin_unlock_irq(&tg->td->lock);
 	ret = 0;
+	goto unprep;
+unlock:
+	spin_unlock_irq(&tg->td->lock);
 unprep:
 	blkg_conf_unprep(&ctx);
 close_bdev:
 	blkg_conf_close_bdev(&ctx);
 	return ret ?: nbytes;
@@ -1634,10 +1649,32 @@ static void throtl_shutdown_wq(struct request_queue *q)
 	struct throtl_data *td = q->td;
 
 	cancel_work_sync(&td->dispatch_work);
 }
 
+static void throtl_shutdown_timers(struct request_queue *q)
+{
+	struct throtl_data *td = q->td;
+	struct blkcg_gq *blkg;
+
+	/*
+	 * blkg_destroy_all() has already offlined the policy, but blkg policy
+	 * data is freed asynchronously.  Shut down per-group timers before
+	 * freeing td, as their callbacks still dereference tg->td.
+	 */
+	mutex_lock(&q->blkcg_mutex);
+	list_for_each_entry(blkg, &q->blkg_list, q_node) {
+		struct throtl_grp *tg = blkg_to_tg(blkg);
+
+		if (tg)
+			timer_shutdown_sync(&tg->service_queue.pending_timer);
+	}
+	mutex_unlock(&q->blkcg_mutex);
+
+	timer_shutdown_sync(&td->service_queue.pending_timer);
+}
+
 static void tg_flush_bios(struct throtl_grp *tg)
 {
 	struct throtl_service_queue *sq = &tg->service_queue;
 
 	if (tg->flags & THROTL_TG_CANCELING)
@@ -1667,11 +1704,17 @@ static void tg_flush_bios(struct throtl_grp *tg)
 	throtl_schedule_next_dispatch(sq->parent_sq, true);
 }
 
 static void throtl_pd_offline(struct blkg_policy_data *pd)
 {
-	tg_flush_bios(pd_to_tg(pd));
+	struct throtl_grp *tg = pd_to_tg(pd);
+	struct throtl_data *td = tg->td;
+	unsigned long flags;
+
+	spin_lock_irqsave(&td->lock, flags);
+	tg_flush_bios(tg);
+	spin_unlock_irqrestore(&td->lock, flags);
 }
 
 struct blkcg_policy blkcg_policy_throtl = {
 	.dfl_cftypes		= throtl_files,
 	.legacy_cftypes		= throtl_legacy_files,
@@ -1723,19 +1766,21 @@ static void tg_cancel_writeback_bios(struct throtl_grp *tg,
 }
 
 void blk_throtl_cancel_bios(struct gendisk *disk)
 {
 	struct request_queue *q = disk->queue;
+	struct throtl_data *td = q->td;
 	struct cgroup_subsys_state *pos_css;
 	struct blkcg_gq *blkg;
 	struct bio_list cancel_bios[2] = { };
 	int rw;
 
 	if (!blk_throtl_activated(q))
 		return;
 
 	spin_lock_irq(&q->queue_lock);
+	spin_lock(&td->lock);
 	/*
 	 * queue_lock is held, rcu lock is not needed here technically.
 	 * However, rcu lock is still held to emphasize that following
 	 * path need RCU protection and to prevent warning from lockdep.
 	 */
@@ -1750,10 +1795,11 @@ void blk_throtl_cancel_bios(struct gendisk *disk)
 		 * del_gendisk.
 		 */
 		tg_cancel_writeback_bios(blkg_to_tg(blkg), cancel_bios);
 	}
 	rcu_read_unlock();
+	spin_unlock(&td->lock);
 	spin_unlock_irq(&q->queue_lock);
 
 	for (rw = READ; rw <= WRITE; rw++) {
 		struct bio *bio;
 		while ((bio = bio_list_pop(&cancel_bios[rw])))
@@ -1789,21 +1835,20 @@ static bool tg_within_limit(struct throtl_grp *tg, struct bio *bio, bool rw)
 	return tg_dispatch_time(tg, bio) == 0;
 }
 
 bool __blk_throtl_bio(struct bio *bio)
 {
-	struct request_queue *q = bdev_get_queue(bio->bi_bdev);
 	struct blkcg_gq *blkg = bio_blkg(bio);
 	struct throtl_qnode *qn = NULL;
 	struct throtl_grp *tg = blkg_to_tg(blkg);
 	struct throtl_service_queue *sq;
 	bool rw = bio_data_dir(bio);
 	bool throttled = false;
 	struct throtl_data *td = tg->td;
 
 	rcu_read_lock();
-	spin_lock_irq(&q->queue_lock);
+	spin_lock_irq(&td->lock);
 	sq = &tg->service_queue;
 
 	while (true) {
 		if (tg_within_limit(tg, bio, rw)) {
 			/* within limits, let's charge and dispatch directly */
@@ -1875,30 +1920,32 @@ bool __blk_throtl_bio(struct bio *bio)
 		tg_update_disptime(tg);
 		throtl_schedule_next_dispatch(tg->service_queue.parent_sq, true);
 	}
 
 out_unlock:
-	spin_unlock_irq(&q->queue_lock);
+	spin_unlock_irq(&td->lock);
 
 	rcu_read_unlock();
 	return throttled;
 }
 
 void blk_throtl_exit(struct gendisk *disk)
 {
 	struct request_queue *q = disk->queue;
+	struct throtl_data *td = q->td;
 
 	/*
 	 * blkg_destroy_all() already deactivate throtl policy, just check and
 	 * free throtl data.
 	 */
-	if (!q->td)
+	if (!td)
 		return;
 
-	timer_delete_sync(&q->td->service_queue.pending_timer);
+	throtl_shutdown_timers(q);
 	throtl_shutdown_wq(q);
-	kfree(q->td);
+	q->td = NULL;
+	kfree(td);
 }
 
 static int __init throtl_init(void)
 {
 	kthrotld_workqueue = alloc_workqueue("kthrotld", WQ_MEM_RECLAIM | WQ_PERCPU, 0);
-- 
2.51.0

[RFC PATCH v3 3/6] blk-cgroup: protect blkgs with blkcg_mutex

[Yu Kuai]

From: Yu Kuai <yukuai@fygo.io>

queue_lock is still needed by block core users, but blkcg no longer
needs it for blkg topology now that throttle runtime state has a private
lock.

Move queue-local blkg topology synchronization to q->blkcg_mutex. Hold
it while creating and destroying blkgs, while preparing and undoing
configuration, and while activating or deactivating policies.

Keep the common bio_blkg() lookup on an RCU fast path so I/O for an
existing blkg does not acquire blkcg_mutex. Only take the mutex when the
blkg hierarchy needs to be created.

Update the BFQ, iocost, iolatency and throttle paths which walk
q->blkg_list or access per-blkg policy state to use the same lock.

blkcg->lock still protects blkcg-local list updates. Some lookups under
blkcg_mutex can race with blkcg updates done for other queues, so keep
those lookups in RCU read-side critical sections. In particular, protect
the parent lookup in blkg_create() and the parent walk in
blkg_lookup_create().

Signed-off-by: Yu Kuai <yukuai@fygo.io>
---
 block/bfq-cgroup.c    |  10 ++-
 block/blk-cgroup.c    | 153 +++++++++++++++++-------------------------
 block/blk-cgroup.h    |  11 ++-
 block/blk-iocost.c    |   8 ++-
 block/blk-iolatency.c |   7 +-
 block/blk-throttle.c  |  10 +--
 6 files changed, 87 insertions(+), 112 deletions(-)

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index 4a3975f9ff74..d64cea475d7b 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -426,11 +426,11 @@ static void bfqg_stats_xfer_dead(struct bfq_group *bfqg)
 	if (!bfqg) /* root_group */
 		return;
 
 	parent = bfqg_parent(bfqg);
 
-	lockdep_assert_held(&bfqg_to_blkg(bfqg)->q->queue_lock);
+	lockdep_assert_held(&bfqg_to_blkg(bfqg)->q->blkcg_mutex);
 
 	if (unlikely(!parent))
 		return;
 
 	bfqg_stats_add_aux(&parent->stats, &bfqg->stats);
@@ -876,11 +876,11 @@ static void bfq_reparent_active_queues(struct bfq_data *bfqd,
 /**
  * bfq_pd_offline - deactivate the entity associated with @pd,
  *		    and reparent its children entities.
  * @pd: descriptor of the policy going offline.
  *
- * blkio already grabs the queue_lock for us, so no need to use
+ * blkio already grabs the blkcg_mutex for us, so no need to use
  * RCU-based magic
  */
 static void bfq_pd_offline(struct blkg_policy_data *pd)
 {
 	struct bfq_service_tree *st;
@@ -949,22 +949,20 @@ void bfq_end_wr_async(struct bfq_data *bfqd)
 {
 	struct request_queue *q = bfqd->queue;
 	struct blkcg_gq *blkg;
 
 	mutex_lock(&q->blkcg_mutex);
-	spin_lock_irq(&q->queue_lock);
-	spin_lock(&bfqd->lock);
+	spin_lock_irq(&bfqd->lock);
 
 	list_for_each_entry(blkg, &q->blkg_list, q_node) {
 		struct bfq_group *bfqg = blkg_to_bfqg(blkg);
 
 		bfq_end_wr_async_queues(bfqd, bfqg);
 	}
 	bfq_end_wr_async_queues(bfqd, bfqd->root_group);
 
-	spin_unlock(&bfqd->lock);
-	spin_unlock_irq(&q->queue_lock);
+	spin_unlock_irq(&bfqd->lock);
 	mutex_unlock(&q->blkcg_mutex);
 }
 
 static int bfq_io_show_weight_legacy(struct seq_file *sf, void *v)
 {
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 5b51be2fefc1..0f34a80a726d 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -62,12 +62,10 @@ static LIST_HEAD(all_blkcgs);		/* protected by blkcg_pol_mutex */
 
 bool blkcg_debug_stats = false;
 
 static DEFINE_RAW_SPINLOCK(blkg_stat_lock);
 
-#define BLKG_DESTROY_BATCH_SIZE  64
-
 const struct rhashtable_params blkg_hash_params = {
 	.key_len		= sizeof_field(struct blkcg_gq, blkcg_id),
 	.key_offset		= offsetof(struct blkcg_gq, blkcg_id),
 	.head_offset		= offsetof(struct blkcg_gq, q_hash_node),
 	.automatic_shrinking	= true,
@@ -139,15 +137,13 @@ static void blkg_free_workfn(struct work_struct *work)
 	for (i = 0; i < BLKCG_MAX_POLS; i++)
 		if (blkg->pd[i])
 			blkcg_policy[i]->pd_free_fn(blkg->pd[i]);
 	if (blkg->parent)
 		blkg_put(blkg->parent);
-	spin_lock_irq(&q->queue_lock);
 	list_del_init(&blkg->q_node);
 	if (list_empty(&q->blkg_list))
 		wake_up_var(&q->blkg_list);
-	spin_unlock_irq(&q->queue_lock);
 	mutex_unlock(&q->blkcg_mutex);
 
 	/*
 	 * Release blkcg css ref only after blkg is removed from q->blkg_list,
 	 * so concurrent iterators won't see a blkg with a freed blkcg.
@@ -397,11 +393,11 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg, struct gendisk *disk,
 				    struct blkcg_gq *new_blkg)
 {
 	struct blkcg_gq *blkg;
 	int i, ret;
 
-	lockdep_assert_held(&disk->queue->queue_lock);
+	lockdep_assert_held(&disk->queue->blkcg_mutex);
 
 	/* request_queue is dying, do not create/recreate a blkg */
 	if (blk_queue_dying(disk->queue)) {
 		ret = -ENODEV;
 		goto err_free_blkg;
@@ -417,16 +413,19 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg, struct gendisk *disk,
 	}
 	blkg = new_blkg;
 
 	/* link parent */
 	if (blkcg_parent(blkcg)) {
+		rcu_read_lock();
 		blkg->parent = blkg_lookup(blkcg_parent(blkcg), disk->queue);
 		if (WARN_ON_ONCE(!blkg->parent)) {
+			rcu_read_unlock();
 			ret = -ENODEV;
 			goto err_free_blkg;
 		}
 		blkg_get(blkg->parent);
+		rcu_read_unlock();
 	}
 
 	/* invoke per-policy init */
 	for (i = 0; i < BLKCG_MAX_POLS; i++) {
 		struct blkcg_policy *pol = blkcg_policy[i];
@@ -434,11 +433,11 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg, struct gendisk *disk,
 		if (blkg->pd[i] && pol->pd_init_fn)
 			pol->pd_init_fn(blkg->pd[i]);
 	}
 
 	/* insert */
-	spin_lock(&blkcg->lock);
+	spin_lock_irq(&blkcg->lock);
 	ret = rhashtable_insert_fast(&disk->queue->blkg_hash,
 				     &blkg->q_hash_node, blkg_hash_params);
 	if (likely(!ret)) {
 		hlist_add_head_rcu(&blkg->blkcg_node, &blkcg->blkg_list);
 		list_add(&blkg->q_node, &disk->queue->blkg_list);
@@ -452,11 +451,11 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg, struct gendisk *disk,
 				blkg->pd[i]->online = true;
 			}
 		}
 		blkg->online = true;
 	}
-	spin_unlock(&blkcg->lock);
+	spin_unlock_irq(&blkcg->lock);
 
 	if (!ret)
 		return blkg;
 
 	/* @blkg failed fully initialized, use the usual release path */
@@ -485,13 +484,12 @@ static struct blkcg_gq *blkg_lookup_tryget(struct blkcg_gq *blkg)
  * @blkcg: blkcg of interest
  * @disk: gendisk of interest
  *
  * Lookup blkg for the @blkcg - @disk pair.  If it doesn't exist, try to
  * create one.  blkg creation is performed recursively from blkcg_root such
- * that all non-root blkg's have access to the parent blkg.
- *
- * Must be called with @disk->queue->queue_lock held.
+ * that all non-root blkg's have access to the parent blkg.  This function
+ * must be called with @disk->queue->blkcg_mutex held.
  *
  * Returns the closest blkg with an extra reference acquired.  If
  * blkg_create() fails while walking down from root, the returned blkg may
  * belong to an ancestor of @blkcg.  This function never returns %NULL.
  */
@@ -499,11 +497,11 @@ static struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,
 		struct gendisk *disk)
 {
 	struct request_queue *q = disk->queue;
 	struct blkcg_gq *blkg;
 
-	lockdep_assert_held(&q->queue_lock);
+	lockdep_assert_held(&q->blkcg_mutex);
 
 	rcu_read_lock();
 	blkg = blkg_lookup(blkcg, q);
 	if (blkg) {
 		blkg = blkg_lookup_tryget(blkg);
@@ -520,20 +518,22 @@ static struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,
 	while (true) {
 		struct blkcg *pos = blkcg;
 		struct blkcg *parent = blkcg_parent(blkcg);
 		struct blkcg_gq *ret_blkg = q->root_blkg;
 
+		rcu_read_lock();
 		while (parent) {
 			blkg = blkg_lookup(parent, q);
 			if (blkg) {
 				/* remember closest blkg */
 				ret_blkg = blkg;
 				break;
 			}
 			pos = parent;
 			parent = blkcg_parent(parent);
 		}
+		rcu_read_unlock();
 
 		blkg = blkg_create(pos, disk, NULL);
 		if (IS_ERR(blkg)) {
 			blkg = ret_blkg;
 			break;
@@ -548,11 +548,11 @@ static struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,
 static void blkg_destroy(struct blkcg_gq *blkg)
 {
 	struct blkcg *blkcg = blkg->blkcg;
 	int i;
 
-	lockdep_assert_held(&blkg->q->queue_lock);
+	lockdep_assert_held(&blkg->q->blkcg_mutex);
 	lockdep_assert_held(&blkcg->lock);
 
 	/*
 	 * blkg stays on the queue list until blkg_free_workfn(), see details in
 	 * blkg_free_workfn(), hence this function can be called from
@@ -585,37 +585,22 @@ static void blkg_destroy(struct blkcg_gq *blkg)
 
 static void blkg_destroy_all(struct gendisk *disk)
 {
 	struct request_queue *q = disk->queue;
 	struct blkcg_gq *blkg;
-	int count = BLKG_DESTROY_BATCH_SIZE;
 	int i;
 
-restart:
 	mutex_lock(&q->blkcg_mutex);
-	spin_lock_irq(&q->queue_lock);
 	list_for_each_entry(blkg, &q->blkg_list, q_node) {
 		struct blkcg *blkcg = blkg->blkcg;
 
 		if (hlist_unhashed(&blkg->blkcg_node))
 			continue;
 
-		spin_lock(&blkcg->lock);
+		spin_lock_irq(&blkcg->lock);
 		blkg_destroy(blkg);
-		spin_unlock(&blkcg->lock);
-
-		/*
-		 * in order to avoid holding the spin lock for too long, release
-		 * it when a batch of blkgs are destroyed.
-		 */
-		if (!(--count)) {
-			count = BLKG_DESTROY_BATCH_SIZE;
-			spin_unlock_irq(&q->queue_lock);
-			mutex_unlock(&q->blkcg_mutex);
-			cond_resched();
-			goto restart;
-		}
+		spin_unlock_irq(&blkcg->lock);
 	}
 
 	/*
 	 * Mark policy deactivated since policy offline has been done, and
 	 * the free is scheduled, so future blkcg_deactivate_policy() can
@@ -627,11 +612,10 @@ static void blkg_destroy_all(struct gendisk *disk)
 		if (pol)
 			__clear_bit(pol->plid, q->blkcg_pols);
 	}
 
 	q->root_blkg = NULL;
-	spin_unlock_irq(&q->queue_lock);
 	mutex_unlock(&q->blkcg_mutex);
 }
 
 static void blkg_iostat_set(struct blkg_iostat *dst, struct blkg_iostat *src)
 {
@@ -843,12 +827,12 @@ EXPORT_SYMBOL_GPL(blkg_conf_open_bdev);
  * accordingly. On success, @ctx->body points to the part of @ctx->input
  * following MAJ:MIN, @ctx->bdev points to the target block device and
  * @ctx->blkg to the blkg being configured.
  *
  * blkg_conf_open_bdev() must be called on @ctx beforehand. On success, this
- * function returns with queue lock held and must be followed by
- * blkg_conf_close_bdev().
+ * function returns with blkcg_mutex held and must be followed by
+ * blkg_conf_unprep().
  */
 int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 		   struct blkg_conf_ctx *ctx)
 {
 	struct gendisk *disk;
@@ -862,18 +846,19 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 	disk = ctx->bdev->bd_disk;
 	q = disk->queue;
 
 	/* Prevent concurrent with blkcg_deactivate_policy() */
 	mutex_lock(&q->blkcg_mutex);
-	spin_lock_irq(&q->queue_lock);
 
 	if (!blkcg_policy_enabled(q, pol)) {
 		ret = -EOPNOTSUPP;
 		goto fail_unlock;
 	}
 
+	rcu_read_lock();
 	blkg = blkg_lookup(blkcg, q);
+	rcu_read_unlock();
 	if (blkg)
 		goto success;
 
 	/*
 	 * Create blkgs walking down from blkcg_root to @blkcg, so that all
@@ -883,33 +868,32 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 		struct blkcg *pos = blkcg;
 		struct blkcg *parent;
 		struct blkcg_gq *new_blkg;
 
 		parent = blkcg_parent(blkcg);
+		rcu_read_lock();
 		while (parent && !blkg_lookup(parent, q)) {
 			pos = parent;
 			parent = blkcg_parent(parent);
 		}
-
-		/* Drop locks to do new blkg allocation with GFP_KERNEL. */
-		spin_unlock_irq(&q->queue_lock);
+		rcu_read_unlock();
 
 		new_blkg = blkg_alloc(pos, disk, GFP_NOIO);
 		if (unlikely(!new_blkg)) {
 			ret = -ENOMEM;
-			goto fail_exit;
+			goto fail_unlock;
 		}
 
-		spin_lock_irq(&q->queue_lock);
-
 		if (!blkcg_policy_enabled(q, pol)) {
 			blkg_free(new_blkg);
 			ret = -EOPNOTSUPP;
 			goto fail_unlock;
 		}
 
+		rcu_read_lock();
 		blkg = blkg_lookup(pos, q);
+		rcu_read_unlock();
 		if (blkg) {
 			blkg_free(new_blkg);
 		} else {
 			blkg = blkg_create(pos, disk, new_blkg);
 			if (IS_ERR(blkg)) {
@@ -920,17 +904,14 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 
 		if (pos == blkcg)
 			goto success;
 	}
 success:
-	mutex_unlock(&q->blkcg_mutex);
 	ctx->blkg = blkg;
 	return 0;
 
 fail_unlock:
-	spin_unlock_irq(&q->queue_lock);
-fail_exit:
 	mutex_unlock(&q->blkcg_mutex);
 	/*
 	 * If queue was bypassing, we should retry.  Do so after a
 	 * short msleep().  It isn't strictly necessary but queue
 	 * can be bypassing for some time and it's always nice to
@@ -949,11 +930,11 @@ EXPORT_SYMBOL_GPL(blkg_conf_prep);
  * @ctx: blkg_conf_ctx initialized with blkg_conf_init()
  */
 void blkg_conf_unprep(struct blkg_conf_ctx *ctx)
 {
 	WARN_ON_ONCE(!ctx->blkg);
-	spin_unlock_irq(&ctx->bdev->bd_disk->queue->queue_lock);
+	mutex_unlock(&ctx->bdev->bd_disk->queue->blkcg_mutex);
 	ctx->blkg = NULL;
 }
 EXPORT_SYMBOL_GPL(blkg_conf_unprep);
 
 /**
@@ -1269,12 +1250,13 @@ static struct blkcg_gq *blkcg_get_first_blkg(struct blkcg *blkcg)
 
 /**
  * blkcg_destroy_blkgs - responsible for shooting down blkgs
  * @blkcg: blkcg of interest
  *
- * blkgs should be removed while holding both q and blkcg locks.  As blkcg lock
- * is nested inside q lock, this function performs reverse double lock dancing.
+ * blkgs should be removed while holding both q->blkcg_mutex and blkcg->lock.
+ * As blkcg->lock is nested inside q->blkcg_mutex, this function performs
+ * reverse double lock dancing.
  * Destroying the blkgs releases the reference held on the blkcg's css allowing
  * blkcg_css_free to eventually be called.
  *
  * This is the blkcg counterpart of ioc_release_fn().
  */
@@ -1285,17 +1267,17 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
 	might_sleep();
 
 	while ((blkg = blkcg_get_first_blkg(blkcg))) {
 		struct request_queue *q = blkg->q;
 
-		spin_lock_irq(&q->queue_lock);
-		spin_lock(&blkcg->lock);
+		mutex_lock(&q->blkcg_mutex);
+		spin_lock_irq(&blkcg->lock);
 
 		blkg_destroy(blkg);
 
-		spin_unlock(&blkcg->lock);
-		spin_unlock_irq(&q->queue_lock);
+		spin_unlock_irq(&blkcg->lock);
+		mutex_unlock(&q->blkcg_mutex);
 
 		blkg_put(blkg);
 		cond_resched();
 	}
 }
@@ -1499,22 +1481,21 @@ int blkcg_init_disk(struct gendisk *disk)
 	new_blkg = blkg_alloc(&blkcg_root, disk, GFP_KERNEL);
 	if (!new_blkg)
 		return -ENOMEM;
 
 	/* Make sure the root blkg exists. */
-	/* spin_lock_irq can serve as RCU read-side critical section. */
-	spin_lock_irq(&q->queue_lock);
+	mutex_lock(&q->blkcg_mutex);
 	blkg = blkg_create(&blkcg_root, disk, new_blkg);
 	if (IS_ERR(blkg))
 		goto err_unlock;
 	q->root_blkg = blkg;
-	spin_unlock_irq(&q->queue_lock);
+	mutex_unlock(&q->blkcg_mutex);
 
 	return 0;
 
 err_unlock:
-	spin_unlock_irq(&q->queue_lock);
+	mutex_unlock(&q->blkcg_mutex);
 	return PTR_ERR(blkg);
 }
 
 void blkcg_exit_disk(struct gendisk *disk)
 {
@@ -1548,51 +1529,37 @@ struct cgroup_subsys io_cgrp_subsys = {
 	.depends_on = 1 << memory_cgrp_id,
 #endif
 };
 EXPORT_SYMBOL_GPL(io_cgrp_subsys);
 
-static struct blkg_policy_data *
-blkcg_policy_detach_pd(struct request_queue *q,
-		       const struct blkcg_policy *pol)
+/*
+ * Tear down per-blkg policy data for @pol on @q.
+ */
+static void blkcg_policy_teardown_pds(struct request_queue *q,
+				      const struct blkcg_policy *pol)
 {
-	struct blkg_policy_data *pd = NULL;
 	struct blkcg_gq *blkg;
 
 	lockdep_assert_held(&q->blkcg_mutex);
 
-	spin_lock_irq(&q->queue_lock);
 	list_for_each_entry(blkg, &q->blkg_list, q_node) {
 		struct blkcg *blkcg = blkg->blkcg;
+		struct blkg_policy_data *pd;
 
-		spin_lock(&blkcg->lock);
+		spin_lock_irq(&blkcg->lock);
 		pd = blkg->pd[pol->plid];
 		if (pd) {
 			if (pd->online && pol->pd_offline_fn)
 				pol->pd_offline_fn(pd);
 			pd->online = false;
 			WRITE_ONCE(blkg->pd[pol->plid], NULL);
 		}
-		spin_unlock(&blkcg->lock);
+		spin_unlock_irq(&blkcg->lock);
 
 		if (pd)
-			break;
+			pol->pd_free_fn(pd);
 	}
-	spin_unlock_irq(&q->queue_lock);
-
-	return pd;
-}
-
-/*
- * Tear down per-blkg policy data for @pol on @q.
- */
-static void blkcg_policy_teardown_pds(struct request_queue *q,
-				      const struct blkcg_policy *pol)
-{
-	struct blkg_policy_data *pd;
-
-	while ((pd = blkcg_policy_detach_pd(q, pol)))
-		pol->pd_free_fn(pd);
 }
 
 /**
  * blkcg_activate_policy - activate a blkcg policy on a gendisk
  * @disk: gendisk of interest
@@ -1600,13 +1567,13 @@ static void blkcg_policy_teardown_pds(struct request_queue *q,
  *
  * Activate @pol on @disk.  Requires %GFP_KERNEL context.  @disk goes through
  * bypass mode to populate its blkgs with policy_data for @pol.
  *
  * Activation happens with @disk bypassed, so nobody would be accessing blkgs
- * from IO path.  Update of each blkg is protected by both queue and blkcg
- * locks so that holding either lock and testing blkcg_policy_enabled() is
- * always enough for dereferencing policy data.
+ * from IO path.  Update of each blkg is protected by q->blkcg_mutex and
+ * blkcg->lock so that holding either lock and testing blkcg_policy_enabled()
+ * is always enough for dereferencing policy data.
  *
  * The caller is responsible for synchronizing [de]activations and policy
  * [un]registerations.  Returns 0 on success, -errno on failure.
  */
 int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
@@ -1631,12 +1598,10 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
 	if (queue_is_mq(q))
 		memflags = blk_mq_freeze_queue(q);
 
 	mutex_lock(&q->blkcg_mutex);
 retry:
-	spin_lock_irq(&q->queue_lock);
-
 	/* blkg_list is pushed at the head, reverse walk to initialize parents first */
 	list_for_each_entry_reverse(blkg, &q->blkg_list, q_node) {
 		struct blkg_policy_data *pd;
 
 		if (blkg->pd[pol->plid])
@@ -1661,23 +1626,24 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
 			if (pinned_blkg)
 				blkg_put(pinned_blkg);
 			blkg_get(blkg);
 			pinned_blkg = blkg;
 
-			spin_unlock_irq(&q->queue_lock);
+			mutex_unlock(&q->blkcg_mutex);
 
 			if (pd_prealloc)
 				pol->pd_free_fn(pd_prealloc);
 			pd_prealloc = pol->pd_alloc_fn(disk, blkg->blkcg,
 						       GFP_KERNEL);
+			mutex_lock(&q->blkcg_mutex);
 			if (pd_prealloc)
 				goto retry;
 			else
 				goto enomem;
 		}
 
-		spin_lock(&blkg->blkcg->lock);
+		spin_lock_irq(&blkg->blkcg->lock);
 
 		pd->blkg = blkg;
 		pd->plid = pol->plid;
 		WRITE_ONCE(blkg->pd[pol->plid], pd);
 
@@ -1686,17 +1652,16 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
 
 		if (pol->pd_online_fn)
 			pol->pd_online_fn(pd);
 		pd->online = true;
 
-		spin_unlock(&blkg->blkcg->lock);
+		spin_unlock_irq(&blkg->blkcg->lock);
 	}
 
 	__set_bit(pol->plid, q->blkcg_pols);
 	ret = 0;
 
-	spin_unlock_irq(&q->queue_lock);
 out:
 	mutex_unlock(&q->blkcg_mutex);
 	if (queue_is_mq(q))
 		blk_mq_unfreeze_queue(q, memflags);
 	if (pinned_blkg)
@@ -1732,15 +1697,12 @@ void blkcg_deactivate_policy(struct gendisk *disk,
 
 	if (queue_is_mq(q))
 		memflags = blk_mq_freeze_queue(q);
 
 	mutex_lock(&q->blkcg_mutex);
-	spin_lock_irq(&q->queue_lock);
 
 	__clear_bit(pol->plid, q->blkcg_pols);
-	spin_unlock_irq(&q->queue_lock);
-
 	blkcg_policy_teardown_pds(q, pol);
 	mutex_unlock(&q->blkcg_mutex);
 
 	if (queue_is_mq(q))
 		blk_mq_unfreeze_queue(q, memflags);
@@ -2162,23 +2124,34 @@ struct blkcg_gq *bio_blkg(struct bio *bio)
 {
 	struct blkcg *blkcg = bio_blkcg(bio);
 	struct gendisk *disk;
 	struct request_queue *q;
 	struct blkcg_gq *blkg;
+	int ret;
 
 	if (!blkcg || !bio->bi_bdev)
 		return NULL;
 
 	if (bio_flagged(bio, BIO_BLKG_REF))
 		return bio_pinned_blkg(bio);
 
 	disk = bio->bi_bdev->bd_disk;
 	q = disk->queue;
 
-	spin_lock_irq(&q->queue_lock);
+	rcu_read_lock();
+	blkg = blkg_lookup(blkcg, q);
+	if (blkg)
+		blkg = blkg_lookup_tryget(blkg);
+	rcu_read_unlock();
+	if (blkg) {
+		bio_set_blkg_ref(bio, blkg);
+		return blkg;
+	}
+
+	mutex_lock(&q->blkcg_mutex);
 	blkg = blkg_lookup_create(blkcg, disk);
-	spin_unlock_irq(&q->queue_lock);
+	mutex_unlock(&q->blkcg_mutex);
 
 	bio_set_blkg_ref(bio, blkg);
 	return blkg;
 }
 EXPORT_SYMBOL_GPL(bio_blkg);
diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index 1925420154c1..dcba4eda9826 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -68,11 +68,11 @@ struct blkcg_gq {
 	struct blkcg_gq			*parent;
 
 	/* reference count */
 	struct percpu_ref		refcnt;
 
-	/* is this blkg online? protected by both blkcg and q locks */
+	/* is this blkg online? protected by blkcg->lock and q->blkcg_mutex */
 	bool				online;
 
 	struct blkg_iostat_set __percpu	*iostat_cpu;
 	struct blkg_iostat_set		iostat;
 
@@ -229,13 +229,13 @@ struct blkg_conf_ctx {
 void blkg_conf_init(struct blkg_conf_ctx *ctx, char *input);
 int blkg_conf_open_bdev(struct blkg_conf_ctx *ctx)
 	__cond_acquires(0, &ctx->bdev->bd_queue->rq_qos_mutex);
 int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 		   struct blkg_conf_ctx *ctx)
-	__cond_acquires(0, &ctx->bdev->bd_disk->queue->queue_lock);
+	__cond_acquires(0, &ctx->bdev->bd_disk->queue->blkcg_mutex);
 void blkg_conf_unprep(struct blkg_conf_ctx *ctx)
-	__releases(ctx->bdev->bd_disk->queue->queue_lock);
+	__releases(ctx->bdev->bd_disk->queue->blkcg_mutex);
 void blkg_conf_close_bdev(struct blkg_conf_ctx *ctx)
 	__releases(&ctx->bdev->bd_queue->rq_qos_mutex);
 
 /**
  * bio_issue_as_root_blkg - see if this bio needs to be issued as root blkg
@@ -388,13 +388,12 @@ static inline void bio_clear_blkcg(struct bio *bio)
  * @d_blkg: loop cursor pointing to the current descendant
  * @pos_css: used for iteration
  * @p_blkg: target blkg to walk descendants of
  *
  * Walk @c_blkg through the descendants of @p_blkg.  Must be used with RCU
- * read locked.  If called under either blkcg or queue lock, the iteration
- * is guaranteed to include all and only online blkgs.  The caller may
- * update @pos_css by calling css_rightmost_descendant() to skip subtree.
+ * read locked.  The caller may update @pos_css by calling
+ * css_rightmost_descendant() to skip subtree.
  * @p_blkg is included in the iteration and the first node to be visited.
  */
 #define blkg_for_each_descendant_pre(d_blkg, pos_css, p_blkg)		\
 	css_for_each_descendant_pre((pos_css), &(p_blkg)->blkcg->css)	\
 		if (((d_blkg) = blkg_lookup(css_to_blkcg(pos_css),	\
diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 57f2b4d4af20..31419add4340 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2773,11 +2773,12 @@ static void ioc_rqos_throttle(struct rq_qos *rqos, struct bio *bio)
 }
 
 static void ioc_rqos_merge(struct rq_qos *rqos, struct request *rq,
 			   struct bio *bio)
 {
-	struct ioc_gq *iocg = blkg_to_iocg(bio_blkg(bio));
+	struct blkcg_gq *blkg = bio_blkg_lookup(rq->bio);
+	struct ioc_gq *iocg = blkg_to_iocg(blkg);
 	struct ioc *ioc = rqos_to_ioc(rqos);
 	sector_t bio_end = bio_end_sector(bio);
 	struct ioc_now now;
 	u64 vtime, abs_cost, cost;
 	unsigned long flags;
@@ -3152,10 +3153,11 @@ static ssize_t ioc_weight_write(struct kernfs_open_file *of, char *buf,
 	struct blkcg *blkcg = css_to_blkcg(of_css(of));
 	struct ioc_cgrp *iocc = blkcg_to_iocc(blkcg);
 	struct blkg_conf_ctx ctx;
 	struct ioc_now now;
 	struct ioc_gq *iocg;
+	unsigned long flags;
 	u32 v;
 	int ret;
 
 	if (!strchr(buf, ':')) {
 		struct blkcg_gq *blkg;
@@ -3204,15 +3206,15 @@ static ssize_t ioc_weight_write(struct kernfs_open_file *of, char *buf,
 			goto unprep;
 		if (v < CGROUP_WEIGHT_MIN || v > CGROUP_WEIGHT_MAX)
 			goto unprep;
 	}
 
-	spin_lock(&iocg->ioc->lock);
+	spin_lock_irqsave(&iocg->ioc->lock, flags);
 	iocg->cfg_weight = v * WEIGHT_ONE;
 	ioc_now(iocg->ioc, &now);
 	weight_updated(iocg, &now);
-	spin_unlock(&iocg->ioc->lock);
+	spin_unlock_irqrestore(&iocg->ioc->lock, flags);
 
 	ret = 0;
 
 unprep:
 	blkg_conf_unprep(&ctx);
diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c
index 7220edafd72b..b4bed73b645b 100644
--- a/block/blk-iolatency.c
+++ b/block/blk-iolatency.c
@@ -640,10 +640,11 @@ static void blkcg_iolatency_exit(struct rq_qos *rqos)
 	struct blk_iolatency *blkiolat = BLKIOLATENCY(rqos);
 
 	timer_shutdown_sync(&blkiolat->timer);
 	flush_work(&blkiolat->enable_work);
 	blkcg_deactivate_policy(rqos->disk, &blkcg_policy_iolatency);
+	flush_work(&blkiolat->enable_work);
 	kfree(blkiolat);
 }
 
 static const struct rq_qos_ops blkcg_iolatency_ops = {
 	.throttle = blkcg_iolatency_throttle,
@@ -812,20 +813,22 @@ static void iolatency_set_min_lat_nsec(struct blkcg_gq *blkg, u64 val)
 static void iolatency_clear_scaling(struct blkcg_gq *blkg)
 {
 	if (blkg->parent) {
 		struct iolatency_grp *iolat = blkg_to_lat(blkg->parent);
 		struct child_latency_info *lat_info;
+		unsigned long flags;
+
 		if (!iolat)
 			return;
 
 		lat_info = &iolat->child_lat;
-		spin_lock(&lat_info->lock);
+		spin_lock_irqsave(&lat_info->lock, flags);
 		atomic_set(&lat_info->scale_cookie, DEFAULT_SCALE_COOKIE);
 		lat_info->last_scale_event = 0;
 		lat_info->scale_grp = NULL;
 		lat_info->scale_lat = 0;
-		spin_unlock(&lat_info->lock);
+		spin_unlock_irqrestore(&lat_info->lock, flags);
 	}
 }
 
 static ssize_t iolatency_set_limit(struct kernfs_open_file *of, char *buf,
 			     size_t nbytes, loff_t off)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 2ff30700e84e..045eeab38646 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1775,14 +1775,14 @@ void blk_throtl_cancel_bios(struct gendisk *disk)
 	int rw;
 
 	if (!blk_throtl_activated(q))
 		return;
 
-	spin_lock_irq(&q->queue_lock);
-	spin_lock(&td->lock);
+	mutex_lock(&q->blkcg_mutex);
+	spin_lock_irq(&td->lock);
 	/*
-	 * queue_lock is held, rcu lock is not needed here technically.
+	 * blkcg_mutex is held, rcu lock is not needed here technically.
 	 * However, rcu lock is still held to emphasize that following
 	 * path need RCU protection and to prevent warning from lockdep.
 	 */
 	rcu_read_lock();
 	blkg_for_each_descendant_post(blkg, pos_css, q->root_blkg) {
@@ -1795,12 +1795,12 @@ void blk_throtl_cancel_bios(struct gendisk *disk)
 		 * del_gendisk.
 		 */
 		tg_cancel_writeback_bios(blkg_to_tg(blkg), cancel_bios);
 	}
 	rcu_read_unlock();
-	spin_unlock(&td->lock);
-	spin_unlock_irq(&q->queue_lock);
+	spin_unlock_irq(&td->lock);
+	mutex_unlock(&q->blkcg_mutex);
 
 	for (rw = READ; rw <= WRITE; rw++) {
 		struct bio *bio;
 		while ((bio = bio_list_pop(&cancel_bios[rw])))
 			bio_io_error(bio);
-- 
2.51.0

[RFC PATCH v3 4/6] blk-cgroup: allocate blkgs in blkg_create

[Yu Kuai]

From: Yu Kuai <yukuai@fygo.io>

Move blkg allocation into blkg_create() and have it take a gfp_t mask, so
that the caller controls whether creation may sleep. blkg_create() now
always allocates the blkg itself instead of sometimes receiving a
preallocated one, which lets the lookup and config paths drop their open-
coded preallocation and retry loops.

blkg_lookup_create() and the root-blkg setup use GFP_NOIO (or GFP_KERNEL
for the root) so they do not recurse into IO reclaim; the nowait policy
path added later will use GFP_ATOMIC.

Signed-off-by: Yu Kuai <yukuai@fygo.io>
---
 block/blk-cgroup.c | 48 +++++++++++++---------------------------------
 1 file changed, 13 insertions(+), 35 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 0f34a80a726d..33fba781017b 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -383,37 +383,29 @@ static struct blkcg_gq *blkg_alloc(struct blkcg *blkcg, struct gendisk *disk,
 out_free_blkg:
 	kfree(blkg);
 	return NULL;
 }
 
-/*
- * If @new_blkg is %NULL, this function tries to allocate a new one as
- * necessary using %GFP_NOWAIT.  @new_blkg is always consumed on return.
- */
 static struct blkcg_gq *blkg_create(struct blkcg *blkcg, struct gendisk *disk,
-				    struct blkcg_gq *new_blkg)
+				    gfp_t gfp_mask)
 {
-	struct blkcg_gq *blkg;
+	struct blkcg_gq *blkg = NULL;
 	int i, ret;
 
 	lockdep_assert_held(&disk->queue->blkcg_mutex);
 
 	/* request_queue is dying, do not create/recreate a blkg */
 	if (blk_queue_dying(disk->queue)) {
 		ret = -ENODEV;
 		goto err_free_blkg;
 	}
 
-	/* allocate */
-	if (!new_blkg) {
-		new_blkg = blkg_alloc(blkcg, disk, GFP_NOWAIT);
-		if (unlikely(!new_blkg)) {
-			ret = -ENOMEM;
-			goto err_free_blkg;
-		}
+	blkg = blkg_alloc(blkcg, disk, gfp_mask);
+	if (unlikely(!blkg)) {
+		ret = -ENOMEM;
+		goto err_free_blkg;
 	}
-	blkg = new_blkg;
 
 	/* link parent */
 	if (blkcg_parent(blkcg)) {
 		rcu_read_lock();
 		blkg->parent = blkg_lookup(blkcg_parent(blkcg), disk->queue);
@@ -461,12 +453,12 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg, struct gendisk *disk,
 	/* @blkg failed fully initialized, use the usual release path */
 	percpu_ref_kill(&blkg->refcnt);
 	return ERR_PTR(ret);
 
 err_free_blkg:
-	if (new_blkg)
-		blkg_free(new_blkg);
+	if (blkg)
+		blkg_free(blkg);
 	return ERR_PTR(ret);
 }
 
 /*
  * The root blkg holds a live reference while the disk is active, so walking
@@ -531,11 +523,11 @@ static struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,
 			pos = parent;
 			parent = blkcg_parent(parent);
 		}
 		rcu_read_unlock();
 
-		blkg = blkg_create(pos, disk, NULL);
+		blkg = blkg_create(pos, disk, GFP_NOIO);
 		if (IS_ERR(blkg)) {
 			blkg = ret_blkg;
 			break;
 		}
 		if (pos == blkcg)
@@ -865,39 +857,29 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 	 * non-root blkgs have access to their parents.
 	 */
 	while (true) {
 		struct blkcg *pos = blkcg;
 		struct blkcg *parent;
-		struct blkcg_gq *new_blkg;
 
 		parent = blkcg_parent(blkcg);
 		rcu_read_lock();
 		while (parent && !blkg_lookup(parent, q)) {
 			pos = parent;
 			parent = blkcg_parent(parent);
 		}
 		rcu_read_unlock();
 
-		new_blkg = blkg_alloc(pos, disk, GFP_NOIO);
-		if (unlikely(!new_blkg)) {
-			ret = -ENOMEM;
-			goto fail_unlock;
-		}
-
 		if (!blkcg_policy_enabled(q, pol)) {
-			blkg_free(new_blkg);
 			ret = -EOPNOTSUPP;
 			goto fail_unlock;
 		}
 
 		rcu_read_lock();
 		blkg = blkg_lookup(pos, q);
 		rcu_read_unlock();
-		if (blkg) {
-			blkg_free(new_blkg);
-		} else {
-			blkg = blkg_create(pos, disk, new_blkg);
+		if (!blkg) {
+			blkg = blkg_create(pos, disk, GFP_NOIO);
 			if (IS_ERR(blkg)) {
 				ret = PTR_ERR(blkg);
 				goto fail_unlock;
 			}
 		}
@@ -1466,27 +1448,23 @@ void blkg_exit_queue(struct request_queue *q)
 }
 
 int blkcg_init_disk(struct gendisk *disk)
 {
 	struct request_queue *q = disk->queue;
-	struct blkcg_gq *new_blkg, *blkg;
+	struct blkcg_gq *blkg;
 
 	/*
 	 * If the queue is shared across disk rebind (e.g., SCSI), the
 	 * previous disk's blkcg state is cleaned up asynchronously via
 	 * disk_release() -> blkcg_exit_disk(). Wait for all old blkgs to be
 	 * removed from the queue list before setting up new blkcg state.
 	 */
 	wait_var_event(&q->blkg_list, list_empty_careful(&q->blkg_list));
 
-	new_blkg = blkg_alloc(&blkcg_root, disk, GFP_KERNEL);
-	if (!new_blkg)
-		return -ENOMEM;
-
 	/* Make sure the root blkg exists. */
 	mutex_lock(&q->blkcg_mutex);
-	blkg = blkg_create(&blkcg_root, disk, new_blkg);
+	blkg = blkg_create(&blkcg_root, disk, GFP_KERNEL);
 	if (IS_ERR(blkg))
 		goto err_unlock;
 	q->root_blkg = blkg;
 	mutex_unlock(&q->blkcg_mutex);
 
-- 
2.51.0

[RFC PATCH v3 5/6] blk-cgroup: share blkg creation between lookup and config prep

[Yu Kuai]

From: Yu Kuai <yukuai@fygo.io>

blkg_conf_prep() open-codes the same parent walk and blkg creation that
blkg_lookup_create() now performs. Give blkg_lookup_create() an out
parameter for the created/found blkg and have it report whether the target
blkg was created or found (returning the closest existing blkg in the out
parameter on failure), then have blkg_conf_prep() use the helper and treat
errors as config failures.

This keeps the bio association path's closest-blkg fallback and removes the
duplicate config path loop.

Signed-off-by: Yu Kuai <yukuai@fygo.io>
---
 block/blk-cgroup.c | 77 +++++++++++++---------------------------------
 1 file changed, 21 insertions(+), 56 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 33fba781017b..31afb433ab18 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -473,34 +473,36 @@ static struct blkcg_gq *blkg_lookup_tryget(struct blkcg_gq *blkg)
 
 /**
  * blkg_lookup_create - lookup blkg, try to create one if not there
  * @blkcg: blkcg of interest
  * @disk: gendisk of interest
+ * @gfp_mask: allocation mask to use
+ * @blkgp: out parameter for the target blkg, or closest blkg on failure
  *
  * Lookup blkg for the @blkcg - @disk pair.  If it doesn't exist, try to
  * create one.  blkg creation is performed recursively from blkcg_root such
  * that all non-root blkg's have access to the parent blkg.  This function
  * must be called with @disk->queue->blkcg_mutex held.
  *
- * Returns the closest blkg with an extra reference acquired.  If
- * blkg_create() fails while walking down from root, the returned blkg may
- * belong to an ancestor of @blkcg.  This function never returns %NULL.
+ * On success, *@blkgp points to the target blkg and 0 is returned.  On
+ * failure, *@blkgp points to the closest blkg and the errno is returned.
+ * The returned blkg does not have an extra reference acquired.
  */
-static struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,
-		struct gendisk *disk)
+static int blkg_lookup_create(struct blkcg *blkcg, struct gendisk *disk,
+			      gfp_t gfp_mask, struct blkcg_gq **blkgp)
 {
 	struct request_queue *q = disk->queue;
 	struct blkcg_gq *blkg;
 
 	lockdep_assert_held(&q->blkcg_mutex);
 
 	rcu_read_lock();
 	blkg = blkg_lookup(blkcg, q);
 	if (blkg) {
-		blkg = blkg_lookup_tryget(blkg);
+		*blkgp = blkg;
 		rcu_read_unlock();
-		return blkg;
+		return 0;
 	}
 	rcu_read_unlock();
 
 	/*
 	 * Create blkgs walking down from blkcg_root to @blkcg, so that all
@@ -523,20 +525,20 @@ static struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,
 			pos = parent;
 			parent = blkcg_parent(parent);
 		}
 		rcu_read_unlock();
 
-		blkg = blkg_create(pos, disk, GFP_NOIO);
+		blkg = blkg_create(pos, disk, gfp_mask);
 		if (IS_ERR(blkg)) {
-			blkg = ret_blkg;
-			break;
+			*blkgp = ret_blkg;
+			return PTR_ERR(blkg);
+		}
+		if (pos == blkcg) {
+			*blkgp = blkg;
+			return 0;
 		}
-		if (pos == blkcg)
-			break;
 	}
-
-	return blkg_lookup_tryget(blkg);
 }
 
 static void blkg_destroy(struct blkcg_gq *blkg)
 {
 	struct blkcg *blkcg = blkg->blkcg;
@@ -844,52 +846,14 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 	if (!blkcg_policy_enabled(q, pol)) {
 		ret = -EOPNOTSUPP;
 		goto fail_unlock;
 	}
 
-	rcu_read_lock();
-	blkg = blkg_lookup(blkcg, q);
-	rcu_read_unlock();
-	if (blkg)
-		goto success;
-
-	/*
-	 * Create blkgs walking down from blkcg_root to @blkcg, so that all
-	 * non-root blkgs have access to their parents.
-	 */
-	while (true) {
-		struct blkcg *pos = blkcg;
-		struct blkcg *parent;
-
-		parent = blkcg_parent(blkcg);
-		rcu_read_lock();
-		while (parent && !blkg_lookup(parent, q)) {
-			pos = parent;
-			parent = blkcg_parent(parent);
-		}
-		rcu_read_unlock();
-
-		if (!blkcg_policy_enabled(q, pol)) {
-			ret = -EOPNOTSUPP;
-			goto fail_unlock;
-		}
-
-		rcu_read_lock();
-		blkg = blkg_lookup(pos, q);
-		rcu_read_unlock();
-		if (!blkg) {
-			blkg = blkg_create(pos, disk, GFP_NOIO);
-			if (IS_ERR(blkg)) {
-				ret = PTR_ERR(blkg);
-				goto fail_unlock;
-			}
-		}
+	ret = blkg_lookup_create(blkcg, disk, GFP_NOIO, &blkg);
+	if (ret)
+		goto fail_unlock;
 
-		if (pos == blkcg)
-			goto success;
-	}
-success:
 	ctx->blkg = blkg;
 	return 0;
 
 fail_unlock:
 	mutex_unlock(&q->blkcg_mutex);
@@ -2124,11 +2088,12 @@ struct blkcg_gq *bio_blkg(struct bio *bio)
 		bio_set_blkg_ref(bio, blkg);
 		return blkg;
 	}
 
 	mutex_lock(&q->blkcg_mutex);
-	blkg = blkg_lookup_create(blkcg, disk);
+	blkg_lookup_create(blkcg, disk, GFP_NOIO, &blkg);
+	blkg = blkg_lookup_tryget(blkg);
 	mutex_unlock(&q->blkcg_mutex);
 
 	bio_set_blkg_ref(bio, blkg);
 	return blkg;
 }
-- 
2.51.0

[RFC PATCH v3 6/6] blk-cgroup: make policy blkg creation nowait-safe

[Yu Kuai]

From: Yu Kuai <yukuai@fygo.io>

bio_blkg() is called by blkcg policy paths when they need a queue-local
blkg. Keep that allocation lazy instead of preparing every REQ_NOWAIT bio
from submit_bio_noacct().

If a policy first needs a blkg for a REQ_NOWAIT bio, use mutex_trylock()
and GFP_ATOMIC so the lookup never sleeps. If the mutex cannot be acquired,
look up and pin the closest existing blkg in the hierarchy under RCU. The
creation helper provides the same fallback if atomic allocation fails, so
valid policy I/O always gets a blkg without blocking.

Signed-off-by: Yu Kuai <yukuai@fygo.io>
---
 block/blk-cgroup.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 31afb433ab18..9895d6661070 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -28,10 +28,11 @@
 #include <linux/atomic.h>
 #include <linux/ctype.h>
 #include <linux/resume_user_mode.h>
 #include <linux/psi.h>
 #include <linux/part_stat.h>
+#include <linux/preempt.h>
 #include "blk.h"
 #include "blk-cgroup.h"
 #include "blk-ioprio.h"
 #include "blk-throttle.h"
 
@@ -469,10 +470,24 @@ static struct blkcg_gq *blkg_lookup_tryget(struct blkcg_gq *blkg)
 	while (!blkg_tryget(blkg))
 		blkg = blkg->parent;
 	return blkg;
 }
 
+static struct blkcg_gq *blkg_lookup_closest(struct blkcg *blkcg,
+					    struct request_queue *q)
+{
+	struct blkcg_gq *blkg;
+
+	rcu_read_lock();
+	while (!(blkg = blkg_lookup(blkcg, q)))
+		blkcg = blkcg_parent(blkcg);
+	blkg = blkg_lookup_tryget(blkg);
+	rcu_read_unlock();
+
+	return blkg;
+}
+
 /**
  * blkg_lookup_create - lookup blkg, try to create one if not there
  * @blkcg: blkcg of interest
  * @disk: gendisk of interest
  * @gfp_mask: allocation mask to use
@@ -2066,11 +2081,10 @@ struct blkcg_gq *bio_blkg(struct bio *bio)
 {
 	struct blkcg *blkcg = bio_blkcg(bio);
 	struct gendisk *disk;
 	struct request_queue *q;
 	struct blkcg_gq *blkg;
-	int ret;
 
 	if (!blkcg || !bio->bi_bdev)
 		return NULL;
 
 	if (bio_flagged(bio, BIO_BLKG_REF))
@@ -2087,10 +2101,29 @@ struct blkcg_gq *bio_blkg(struct bio *bio)
 	if (blkg) {
 		bio_set_blkg_ref(bio, blkg);
 		return blkg;
 	}
 
+	if (bio->bi_opf & REQ_NOWAIT) {
+		/*
+		 * Nowait callers must not sleep on the mutex nor allocate with
+		 * sleeping GFPs.  Trylock the mutex and create the missing blkg
+		 * atomically.  If the mutex cannot be acquired, skip allocation
+		 * and pin the closest existing blkg instead.  blkg_lookup_create()
+		 * provides the same fallback if allocation fails.
+		 */
+		if (!preemptible() || !mutex_trylock(&q->blkcg_mutex)) {
+			blkg = blkg_lookup_closest(blkcg, q);
+		} else {
+			blkg_lookup_create(blkcg, disk, GFP_ATOMIC, &blkg);
+			blkg = blkg_lookup_tryget(blkg);
+			mutex_unlock(&q->blkcg_mutex);
+		}
+		bio_set_blkg_ref(bio, blkg);
+		return blkg;
+	}
+
 	mutex_lock(&q->blkcg_mutex);
 	blkg_lookup_create(blkcg, disk, GFP_NOIO, &blkg);
 	blkg = blkg_lookup_tryget(blkg);
 	mutex_unlock(&q->blkcg_mutex);
 
-- 
2.51.0

Re: [RFC PATCH v3 4/6] blk-cgroup: allocate blkgs in blkg_create

[Tao Cui]

Hi Kuai,

在 2026/8/23 23:29, Yu Kuai 写道:
> From: Yu Kuai <yukuai@fygo.io>
> 
> Move blkg allocation into blkg_create() and have it take a gfp_t mask, so
> that the caller controls whether creation may sleep. blkg_create() now
> always allocates the blkg itself instead of sometimes receiving a
> preallocated one, which lets the lookup and config paths drop their open-
> coded preallocation and retry loops.
> 
> blkg_lookup_create() and the root-blkg setup use GFP_NOIO (or GFP_KERNEL
> for the root) so they do not recurse into IO reclaim; the nowait policy
> path added later will use GFP_ATOMIC.
> 
> Signed-off-by: Yu Kuai <yukuai@fygo.io>
> ---
>  block/blk-cgroup.c | 48 +++++++++++++---------------------------------
>  1 file changed, 13 insertions(+), 35 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 0f34a80a726d..33fba781017b 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -383,37 +383,29 @@ static struct blkcg_gq *blkg_alloc(struct blkcg *blkcg, struct gendisk *disk,
>  out_free_blkg:
>  	kfree(blkg);
>  	return NULL;
>  }
>  
> -/*
> - * If @new_blkg is %NULL, this function tries to allocate a new one as
> - * necessary using %GFP_NOWAIT.  @new_blkg is always consumed on return.
> - */
>  static struct blkcg_gq *blkg_create(struct blkcg *blkcg, struct gendisk *disk,
> -				    struct blkcg_gq *new_blkg)
> +				    gfp_t gfp_mask)
>  {
> -	struct blkcg_gq *blkg;
> +	struct blkcg_gq *blkg = NULL;
>  	int i, ret;
>  
>  	lockdep_assert_held(&disk->queue->blkcg_mutex);
>  
>  	/* request_queue is dying, do not create/recreate a blkg */
>  	if (blk_queue_dying(disk->queue)) {
>  		ret = -ENODEV;
>  		goto err_free_blkg;
>  	}
>  

Reading patch 4/6, one thing looked off to me. Before the series,
blkg_create() allocated with GFP_NOWAIT under queue_lock, or took a
caller-preallocated object. After patches 3+4, it allocates with the
passed gfp_mask while holding blkcg_mutex:

> -	/* allocate */
> -	if (!new_blkg) {
> -		new_blkg = blkg_alloc(blkcg, disk, GFP_NOWAIT);
> -		if (unlikely(!new_blkg)) {
> -			ret = -ENOMEM;
> -			goto err_free_blkg;
> -		}
> +	blkg = blkg_alloc(blkcg, disk, gfp_mask);
> +	if (unlikely(!blkg)) {
> +		ret = -ENOMEM;
> +		goto err_free_blkg;
>  	}
> -	blkg = new_blkg;
>  

(patch 4 also makes blkg_lookup_create() pass GFP_NOIO for the
config path, where blkg_conf_prep() used to drop queue_lock before
allocating.)

blk_throtl_init() reaches blkcg_activate_policy() with the queue
frozen, so we can end up allocating (and entering reclaim) while
holding q_usage_counter -> blkcg_mutex. Reclaim can then add the
q_usage_counter edge from the disk probe side.

I booted the series in a VM with PROVE_LOCKDEP to check, and lockdep
does report a circular dependency on the first io.max write, every
boot:

    bash/2330 is trying to acquire lock:
    ffff88810e518548 (&q->blkcg_mutex){+.+.}-{4:4}, at: blkcg_activate_policy+0x1ee/0xa00
    but task is already holding lock:
    ffff88810e518060 (&q->q_usage_counter(io)){++++}-{0:0}, at: blk_mq_freeze_queue_nomemsave+0xd/0x20

    Chain exists of:
    &q->blkcg_mutex --> fs_reclaim --> &q->q_usage_counter

GFP_NOIO keeps direct writeback out of reclaim, so an actual hang
looks hard to hit -- but the splat fires on a very common operation.

Could the two-phase allocation already used for pd_prealloc in
blkcg_activate_policy() also apply to blkg_create()? I.e. try
GFP_NOWAIT under blkcg_mutex, and if that fails drop the mutex,
preallocate with GFP_KERNEL and retry. I think that drops the
blkcg_mutex -> fs_reclaim edge, but I may be missing something.

Thanks,
Tao

>  	/* link parent */
>  	if (blkcg_parent(blkcg)) {
>  		rcu_read_lock();
>  		blkg->parent = blkg_lookup(blkcg_parent(blkcg), disk->queue);
> @@ -461,12 +453,12 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg, struct gendisk *disk,
>  	/* @blkg failed fully initialized, use the usual release path */
>  	percpu_ref_kill(&blkg->refcnt);
>  	return ERR_PTR(ret);
>  
>  err_free_blkg:
> -	if (new_blkg)
> -		blkg_free(new_blkg);
> +	if (blkg)
> +		blkg_free(blkg);
>  	return ERR_PTR(ret);
>  }
>  
>  /*
>   * The root blkg holds a live reference while the disk is active, so walking
> @@ -531,11 +523,11 @@ static struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,
>  			pos = parent;
>  			parent = blkcg_parent(parent);
>  		}
>  		rcu_read_unlock();
>  
> -		blkg = blkg_create(pos, disk, NULL);
> +		blkg = blkg_create(pos, disk, GFP_NOIO);
>  		if (IS_ERR(blkg)) {
>  			blkg = ret_blkg;
>  			break;
>  		}
>  		if (pos == blkcg)
> @@ -865,39 +857,29 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
>  	 * non-root blkgs have access to their parents.
>  	 */
>  	while (true) {
>  		struct blkcg *pos = blkcg;
>  		struct blkcg *parent;
> -		struct blkcg_gq *new_blkg;
>  
>  		parent = blkcg_parent(blkcg);
>  		rcu_read_lock();
>  		while (parent && !blkg_lookup(parent, q)) {
>  			pos = parent;
>  			parent = blkcg_parent(parent);
>  		}
>  		rcu_read_unlock();
>  
> -		new_blkg = blkg_alloc(pos, disk, GFP_NOIO);
> -		if (unlikely(!new_blkg)) {
> -			ret = -ENOMEM;
> -			goto fail_unlock;
> -		}
> -
>  		if (!blkcg_policy_enabled(q, pol)) {
> -			blkg_free(new_blkg);
>  			ret = -EOPNOTSUPP;
>  			goto fail_unlock;
>  		}
>  
>  		rcu_read_lock();
>  		blkg = blkg_lookup(pos, q);
>  		rcu_read_unlock();
> -		if (blkg) {
> -			blkg_free(new_blkg);
> -		} else {
> -			blkg = blkg_create(pos, disk, new_blkg);
> +		if (!blkg) {
> +			blkg = blkg_create(pos, disk, GFP_NOIO);
>  			if (IS_ERR(blkg)) {
>  				ret = PTR_ERR(blkg);
>  				goto fail_unlock;
>  			}
>  		}
> @@ -1466,27 +1448,23 @@ void blkg_exit_queue(struct request_queue *q)
>  }
>  
>  int blkcg_init_disk(struct gendisk *disk)
>  {
>  	struct request_queue *q = disk->queue;
> -	struct blkcg_gq *new_blkg, *blkg;
> +	struct blkcg_gq *blkg;
>  
>  	/*
>  	 * If the queue is shared across disk rebind (e.g., SCSI), the
>  	 * previous disk's blkcg state is cleaned up asynchronously via
>  	 * disk_release() -> blkcg_exit_disk(). Wait for all old blkgs to be
>  	 * removed from the queue list before setting up new blkcg state.
>  	 */
>  	wait_var_event(&q->blkg_list, list_empty_careful(&q->blkg_list));
>  
> -	new_blkg = blkg_alloc(&blkcg_root, disk, GFP_KERNEL);
> -	if (!new_blkg)
> -		return -ENOMEM;
> -
>  	/* Make sure the root blkg exists. */
>  	mutex_lock(&q->blkcg_mutex);
> -	blkg = blkg_create(&blkcg_root, disk, new_blkg);
> +	blkg = blkg_create(&blkcg_root, disk, GFP_KERNEL);
>  	if (IS_ERR(blkg))
>  		goto err_unlock;
>  	q->root_blkg = blkg;
>  	mutex_unlock(&q->blkcg_mutex);
>  

Re: [RFC PATCH v3 4/6] blk-cgroup: allocate blkgs in blkg_create

[yu kuai]

Hi,

在 2026/8/25 9:35, Tao Cui 写道:
> Hi Kuai,
>
> 在 2026/8/23 23:29, Yu Kuai 写道:
>> From: Yu Kuai <yukuai@fygo.io>
>>
>> Move blkg allocation into blkg_create() and have it take a gfp_t mask, so
>> that the caller controls whether creation may sleep. blkg_create() now
>> always allocates the blkg itself instead of sometimes receiving a
>> preallocated one, which lets the lookup and config paths drop their open-
>> coded preallocation and retry loops.
>>
>> blkg_lookup_create() and the root-blkg setup use GFP_NOIO (or GFP_KERNEL
>> for the root) so they do not recurse into IO reclaim; the nowait policy
>> path added later will use GFP_ATOMIC.
>>
>> Signed-off-by: Yu Kuai <yukuai@fygo.io>
>> ---
>>   block/blk-cgroup.c | 48 +++++++++++++---------------------------------
>>   1 file changed, 13 insertions(+), 35 deletions(-)
>>
>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index 0f34a80a726d..33fba781017b 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -383,37 +383,29 @@ static struct blkcg_gq *blkg_alloc(struct blkcg *blkcg, struct gendisk *disk,
>>   out_free_blkg:
>>   	kfree(blkg);
>>   	return NULL;
>>   }
>>   
>> -/*
>> - * If @new_blkg is %NULL, this function tries to allocate a new one as
>> - * necessary using %GFP_NOWAIT.  @new_blkg is always consumed on return.
>> - */
>>   static struct blkcg_gq *blkg_create(struct blkcg *blkcg, struct gendisk *disk,
>> -				    struct blkcg_gq *new_blkg)
>> +				    gfp_t gfp_mask)
>>   {
>> -	struct blkcg_gq *blkg;
>> +	struct blkcg_gq *blkg = NULL;
>>   	int i, ret;
>>   
>>   	lockdep_assert_held(&disk->queue->blkcg_mutex);
>>   
>>   	/* request_queue is dying, do not create/recreate a blkg */
>>   	if (blk_queue_dying(disk->queue)) {
>>   		ret = -ENODEV;
>>   		goto err_free_blkg;
>>   	}
>>   
> Reading patch 4/6, one thing looked off to me. Before the series,
> blkg_create() allocated with GFP_NOWAIT under queue_lock, or took a
> caller-preallocated object. After patches 3+4, it allocates with the
> passed gfp_mask while holding blkcg_mutex:
>
>> -	/* allocate */
>> -	if (!new_blkg) {
>> -		new_blkg = blkg_alloc(blkcg, disk, GFP_NOWAIT);
>> -		if (unlikely(!new_blkg)) {
>> -			ret = -ENOMEM;
>> -			goto err_free_blkg;
>> -		}
>> +	blkg = blkg_alloc(blkcg, disk, gfp_mask);
>> +	if (unlikely(!blkg)) {
>> +		ret = -ENOMEM;
>> +		goto err_free_blkg;
>>   	}
>> -	blkg = new_blkg;
>>   
> (patch 4 also makes blkg_lookup_create() pass GFP_NOIO for the
> config path, where blkg_conf_prep() used to drop queue_lock before
> allocating.)
>
> blk_throtl_init() reaches blkcg_activate_policy() with the queue
> frozen, so we can end up allocating (and entering reclaim) while
> holding q_usage_counter -> blkcg_mutex. Reclaim can then add the
> q_usage_counter edge from the disk probe side.
>
> I booted the series in a VM with PROVE_LOCKDEP to check, and lockdep
> does report a circular dependency on the first io.max write, every
> boot:
>
>      bash/2330 is trying to acquire lock:
>      ffff88810e518548 (&q->blkcg_mutex){+.+.}-{4:4}, at: blkcg_activate_policy+0x1ee/0xa00
>      but task is already holding lock:
>      ffff88810e518060 (&q->q_usage_counter(io)){++++}-{0:0}, at: blk_mq_freeze_queue_nomemsave+0xd/0x20
>
>      Chain exists of:
>      &q->blkcg_mutex --> fs_reclaim --> &q->q_usage_counter
>
> GFP_NOIO keeps direct writeback out of reclaim, so an actual hang
> looks hard to hit -- but the splat fires on a very common operation.

This is a known problem, and there is also a known problem to call pd_alloc_fn() while
queue is frozen, because percpu allocation there can trigger lockdep deadlock as well.

>
> Could the two-phase allocation already used for pd_prealloc in
> blkcg_activate_policy() also apply to blkg_create()? I.e. try
> GFP_NOWAIT under blkcg_mutex, and if that fails drop the mutex,
> preallocate with GFP_KERNEL and retry. I think that drops the
> blkcg_mutex -> fs_reclaim edge, but I may be missing something.

I'm afraid not, this is not the plan to fix the deadlock, the plan for the order is:

1) hold blkcg_mutex first;
2) allocate memory, including blkg and pd_alloc_fn;
3) queue freeze;
4) pd_init_fn and make blkg online;

>
> Thanks,
> Tao
>
>>   	/* link parent */
>>   	if (blkcg_parent(blkcg)) {
>>   		rcu_read_lock();
>>   		blkg->parent = blkg_lookup(blkcg_parent(blkcg), disk->queue);
>> @@ -461,12 +453,12 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg, struct gendisk *disk,
>>   	/* @blkg failed fully initialized, use the usual release path */
>>   	percpu_ref_kill(&blkg->refcnt);
>>   	return ERR_PTR(ret);
>>   
>>   err_free_blkg:
>> -	if (new_blkg)
>> -		blkg_free(new_blkg);
>> +	if (blkg)
>> +		blkg_free(blkg);
>>   	return ERR_PTR(ret);
>>   }
>>   
>>   /*
>>    * The root blkg holds a live reference while the disk is active, so walking
>> @@ -531,11 +523,11 @@ static struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,
>>   			pos = parent;
>>   			parent = blkcg_parent(parent);
>>   		}
>>   		rcu_read_unlock();
>>   
>> -		blkg = blkg_create(pos, disk, NULL);
>> +		blkg = blkg_create(pos, disk, GFP_NOIO);
>>   		if (IS_ERR(blkg)) {
>>   			blkg = ret_blkg;
>>   			break;
>>   		}
>>   		if (pos == blkcg)
>> @@ -865,39 +857,29 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
>>   	 * non-root blkgs have access to their parents.
>>   	 */
>>   	while (true) {
>>   		struct blkcg *pos = blkcg;
>>   		struct blkcg *parent;
>> -		struct blkcg_gq *new_blkg;
>>   
>>   		parent = blkcg_parent(blkcg);
>>   		rcu_read_lock();
>>   		while (parent && !blkg_lookup(parent, q)) {
>>   			pos = parent;
>>   			parent = blkcg_parent(parent);
>>   		}
>>   		rcu_read_unlock();
>>   
>> -		new_blkg = blkg_alloc(pos, disk, GFP_NOIO);
>> -		if (unlikely(!new_blkg)) {
>> -			ret = -ENOMEM;
>> -			goto fail_unlock;
>> -		}
>> -
>>   		if (!blkcg_policy_enabled(q, pol)) {
>> -			blkg_free(new_blkg);
>>   			ret = -EOPNOTSUPP;
>>   			goto fail_unlock;
>>   		}
>>   
>>   		rcu_read_lock();
>>   		blkg = blkg_lookup(pos, q);
>>   		rcu_read_unlock();
>> -		if (blkg) {
>> -			blkg_free(new_blkg);
>> -		} else {
>> -			blkg = blkg_create(pos, disk, new_blkg);
>> +		if (!blkg) {
>> +			blkg = blkg_create(pos, disk, GFP_NOIO);
>>   			if (IS_ERR(blkg)) {
>>   				ret = PTR_ERR(blkg);
>>   				goto fail_unlock;
>>   			}
>>   		}
>> @@ -1466,27 +1448,23 @@ void blkg_exit_queue(struct request_queue *q)
>>   }
>>   
>>   int blkcg_init_disk(struct gendisk *disk)
>>   {
>>   	struct request_queue *q = disk->queue;
>> -	struct blkcg_gq *new_blkg, *blkg;
>> +	struct blkcg_gq *blkg;
>>   
>>   	/*
>>   	 * If the queue is shared across disk rebind (e.g., SCSI), the
>>   	 * previous disk's blkcg state is cleaned up asynchronously via
>>   	 * disk_release() -> blkcg_exit_disk(). Wait for all old blkgs to be
>>   	 * removed from the queue list before setting up new blkcg state.
>>   	 */
>>   	wait_var_event(&q->blkg_list, list_empty_careful(&q->blkg_list));
>>   
>> -	new_blkg = blkg_alloc(&blkcg_root, disk, GFP_KERNEL);
>> -	if (!new_blkg)
>> -		return -ENOMEM;
>> -
>>   	/* Make sure the root blkg exists. */
>>   	mutex_lock(&q->blkcg_mutex);
>> -	blkg = blkg_create(&blkcg_root, disk, new_blkg);
>> +	blkg = blkg_create(&blkcg_root, disk, GFP_KERNEL);
>>   	if (IS_ERR(blkg))
>>   		goto err_unlock;
>>   	q->root_blkg = blkg;
>>   	mutex_unlock(&q->blkcg_mutex);
>>   

-- 
Thanks,
Kuai