[PATCH -next 0/4] blk-iocost: make sure parent iocg is exited before child

[PATCH -next 0/4] blk-iocost: make sure parent iocg is exited before child

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Yu Kuai (4):
  blk-iocost: track whether iocg is still online
  blk-iocost: don't throttle bio if iocg is offlined
  blk-iocost: dispatch all throttled bio in ioc_pd_offline
  blk-iocost: guarantee the exit order of iocg

 block/blk-iocost.c | 58 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 12 deletions(-)

-- 
2.31.1

[PATCH -next 1/4] blk-iocost: track whether iocg is still online

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

blkcg_gq->online can't be used in iocost because it gets cleared only after
all policies are offlined. This patch add a new field 'online' in iocg.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-iocost.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 3aeb8538f0c3..1498879c4a52 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -459,6 +459,8 @@ struct ioc_gq {
 	struct blkg_policy_data		pd;
 	struct ioc			*ioc;
 
+	bool online;
+
 	/*
 	 * A iocg can get its weight from two sources - an explicit
 	 * per-device-cgroup configuration or the default weight of the
@@ -2952,6 +2954,7 @@ static void ioc_pd_init(struct blkg_policy_data *pd)
 	ioc_now(ioc, &now);
 
 	iocg->ioc = ioc;
+	iocg->online = true;
 	atomic64_set(&iocg->vtime, now.vnow);
 	atomic64_set(&iocg->done_vtime, now.vnow);
 	atomic64_set(&iocg->active_period, atomic64_read(&ioc->cur_period));
@@ -2977,6 +2980,19 @@ static void ioc_pd_init(struct blkg_policy_data *pd)
 	spin_unlock_irqrestore(&ioc->lock, flags);
 }
 
+static void ioc_pd_offline(struct blkg_policy_data *pd)
+{
+	struct ioc_gq *iocg = pd_to_iocg(pd);
+	struct ioc *ioc = iocg->ioc;
+	unsigned long flags;
+
+	if (ioc) {
+		spin_lock_irqsave(&ioc->lock, flags);
+		iocg->online = false;
+		spin_unlock_irqrestore(&ioc->lock, flags);
+	}
+}
+
 static void ioc_pd_free(struct blkg_policy_data *pd)
 {
 	struct ioc_gq *iocg = pd_to_iocg(pd);
@@ -3467,6 +3483,7 @@ static struct blkcg_policy blkcg_policy_iocost = {
 	.cpd_free_fn	= ioc_cpd_free,
 	.pd_alloc_fn	= ioc_pd_alloc,
 	.pd_init_fn	= ioc_pd_init,
+	.pd_offline_fn	= ioc_pd_offline,
 	.pd_free_fn	= ioc_pd_free,
 	.pd_stat_fn	= ioc_pd_stat,
 };
-- 
2.31.1

Re: [PATCH -next 1/4] blk-iocost: track whether iocg is still online

[Christoph Hellwig]

On Sat, Dec 17, 2022 at 11:05:24AM +0800, Yu Kuai wrote:
> @@ -459,6 +459,8 @@ struct ioc_gq {
>  	struct blkg_policy_data		pd;
>  	struct ioc			*ioc;
>  
> +	bool online;

Nit: maybe tab align this field like the fields above it.

> +static void ioc_pd_offline(struct blkg_policy_data *pd)
> +{
> +	struct ioc_gq *iocg = pd_to_iocg(pd);
> +	struct ioc *ioc = iocg->ioc;
> +	unsigned long flags;
> +
> +	if (ioc) {

How could ioc be NULL here?

Re: [PATCH -next 1/4] blk-iocost: track whether iocg is still online

[Yu Kuai]

Hi, Christoph

在 2022/12/21 18:33, Christoph Hellwig 写道:
> On Sat, Dec 17, 2022 at 11:05:24AM +0800, Yu Kuai wrote:
>> @@ -459,6 +459,8 @@ struct ioc_gq {
>>   	struct blkg_policy_data		pd;
>>   	struct ioc			*ioc;
>>   
>> +	bool online;
> 
> Nit: maybe tab align this field like the fields above it.
> 
>> +static void ioc_pd_offline(struct blkg_policy_data *pd)
>> +{
>> +	struct ioc_gq *iocg = pd_to_iocg(pd);
>> +	struct ioc *ioc = iocg->ioc;
>> +	unsigned long flags;
>> +
>> +	if (ioc) {
> 
> How could ioc be NULL here?
> 

As I explained in another thread.. pd_offline_fn() can be called without
pd_init_fn(), which is a bug from upper layer...

blkcg_activate_policy
  spin_lock_irq(&q->queue_lock);
  list_for_each_entry_reverse(blkg, &q->blkg_list
   pd_alloc_fn(GFP_NOWAIT | __GFP_NOWARN,...) -> failed

   spin_unlock_irq(&q->queue_lock);
   // release queue_lock here is problematic, this will cause
pd_offline_fn called without pd_init_fn.
   pd_alloc_fn(__GFP_NOWARN,...)

Thanks,
Kuai
> .
> 

[PATCH -next 2/4] blk-iocost: don't throttle bio if iocg is offlined

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

bio will grab blkg reference, however, blkcg->online_pin is not grabbed,
hence cgroup can be removed after thread exit while bio is still in
progress. Bypass io in this case since it doesn't make sense to
throttle bio while cgroup is removed.

This patch also prepare to move operations on iocg from ioc_pd_free()
to ioc_pd_offline().

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-iocost.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 1498879c4a52..23cc734dbe43 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -695,6 +695,20 @@ static struct ioc_cgrp *blkcg_to_iocc(struct blkcg *blkcg)
 			    struct ioc_cgrp, cpd);
 }
 
+static struct ioc_gq *ioc_bio_iocg(struct bio *bio)
+{
+	struct blkcg_gq *blkg = bio->bi_blkg;
+
+	if (blkg && blkg->online) {
+		struct ioc_gq *iocg = blkg_to_iocg(blkg);
+
+		if (iocg && iocg->online)
+			return iocg;
+	}
+
+	return NULL;
+}
+
 /*
  * Scale @abs_cost to the inverse of @hw_inuse.  The lower the hierarchical
  * weight, the more expensive each IO.  Must round up.
@@ -1262,6 +1276,9 @@ static bool iocg_activate(struct ioc_gq *iocg, struct ioc_now *now)
 
 	spin_lock_irq(&ioc->lock);
 
+	if (!iocg->online)
+		goto fail_unlock;
+
 	ioc_now(ioc, now);
 
 	/* update period */
@@ -2561,9 +2578,8 @@ static u64 calc_size_vtime_cost(struct request *rq, struct ioc *ioc)
 
 static void ioc_rqos_throttle(struct rq_qos *rqos, struct bio *bio)
 {
-	struct blkcg_gq *blkg = bio->bi_blkg;
 	struct ioc *ioc = rqos_to_ioc(rqos);
-	struct ioc_gq *iocg = blkg_to_iocg(blkg);
+	struct ioc_gq *iocg = ioc_bio_iocg(bio);
 	struct ioc_now now;
 	struct iocg_wait wait;
 	u64 abs_cost, cost, vtime;
@@ -2697,7 +2713,7 @@ 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->bi_blkg);
+	struct ioc_gq *iocg = ioc_bio_iocg(bio);
 	struct ioc *ioc = rqos_to_ioc(rqos);
 	sector_t bio_end = bio_end_sector(bio);
 	struct ioc_now now;
@@ -2755,7 +2771,7 @@ static void ioc_rqos_merge(struct rq_qos *rqos, struct request *rq,
 
 static void ioc_rqos_done_bio(struct rq_qos *rqos, struct bio *bio)
 {
-	struct ioc_gq *iocg = blkg_to_iocg(bio->bi_blkg);
+	struct ioc_gq *iocg = ioc_bio_iocg(bio);
 
 	if (iocg && bio->bi_iocost_cost)
 		atomic64_add(bio->bi_iocost_cost, &iocg->done_vtime);
-- 
2.31.1

Re: [PATCH -next 2/4] blk-iocost: don't throttle bio if iocg is offlined

[Tejun Heo]

On Sat, Dec 17, 2022 at 11:05:25AM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> bio will grab blkg reference, however, blkcg->online_pin is not grabbed,
> hence cgroup can be removed after thread exit while bio is still in
> progress. Bypass io in this case since it doesn't make sense to
> throttle bio while cgroup is removed.

I don't get it. Why wouldn't that make sense? ISTR some occasions where we
clear the config to mitigate exits stalling for too long but in general a
policy being active on a draining cgroup shouldn't be a problem.

Thanks.

-- 
tejun

Re: [PATCH -next 2/4] blk-iocost: don't throttle bio if iocg is offlined

[Yu Kuai]

Hi,

在 2022/12/20 5:28, Tejun Heo 写道:
> On Sat, Dec 17, 2022 at 11:05:25AM +0800, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> bio will grab blkg reference, however, blkcg->online_pin is not grabbed,
>> hence cgroup can be removed after thread exit while bio is still in
>> progress. Bypass io in this case since it doesn't make sense to
>> throttle bio while cgroup is removed.
> 
> I don't get it. Why wouldn't that make sense? ISTR some occasions where we
> clear the config to mitigate exits stalling for too long but in general a
> policy being active on a draining cgroup shouldn't be a problem.

The main reason here for patch 2/3 is for patch 4, since bio can still
reach rq_qos after pd_offline_fn is called.

Currently, it's not consistent and seems messy how different policies
implement pd_alloc/free_fn, pd_online/offline_fn, and pd_init_fn. For
iocost, iocg is exited in pd_free_fn, and parent iocg can exits before
child, which will cause many problems.

Patch 2/3 are not necessary if we don't choose to fix such problems by
exiting iocg in ioc_pd_offline() in patch 4.

I'll try to think about how to use refcnting, either from blkg layer or
add refcnting for iocg.

Thanks,
Kuai
> 
> Thanks.
> 

[PATCH -next 3/4] blk-iocost: dispatch all throttled bio in ioc_pd_offline

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Currently, if cgroup is removed while some bio is still throttled, such
bio will still wait for timer to dispatch. On the one hand, it
doesn't make sense to throttle bio while cgroup is removed, on the other
hand, this behaviour makes it hard to guarantee the exit order for
iocg(in ioc_pd_free() currently).

This patch make iocg->online updated under both 'ioc->lock' and
'iocg->waitq.lock', so it can be guaranteed that iocg will stay online
while holding any lock. In the meantime, all throttled bio will be
dispatched immediately in ioc_pd_offline().

This patch also prepare to move operations on iocg from ioc_pd_free()
to ioc_pd_offline().

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-iocost.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 23cc734dbe43..b63ecfdd815c 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -1448,14 +1448,18 @@ static int iocg_wake_fn(struct wait_queue_entry *wq_entry, unsigned mode,
 {
 	struct iocg_wait *wait = container_of(wq_entry, struct iocg_wait, wait);
 	struct iocg_wake_ctx *ctx = key;
-	u64 cost = abs_cost_to_cost(wait->abs_cost, ctx->hw_inuse);
 
-	ctx->vbudget -= cost;
+	if (ctx->iocg->online) {
+		u64 cost = abs_cost_to_cost(wait->abs_cost, ctx->hw_inuse);
 
-	if (ctx->vbudget < 0)
-		return -1;
+		ctx->vbudget -= cost;
+
+		if (ctx->vbudget < 0)
+			return -1;
+
+		iocg_commit_bio(ctx->iocg, wait->bio, wait->abs_cost, cost);
+	}
 
-	iocg_commit_bio(ctx->iocg, wait->bio, wait->abs_cost, cost);
 	wait->committed = true;
 
 	/*
@@ -3003,9 +3007,14 @@ static void ioc_pd_offline(struct blkg_policy_data *pd)
 	unsigned long flags;
 
 	if (ioc) {
-		spin_lock_irqsave(&ioc->lock, flags);
+		struct iocg_wake_ctx ctx = { .iocg = iocg };
+
+		iocg_lock(iocg, true, &flags);
 		iocg->online = false;
-		spin_unlock_irqrestore(&ioc->lock, flags);
+		iocg_unlock(iocg, true, &flags);
+
+		hrtimer_cancel(&iocg->waitq_timer);
+		__wake_up(&iocg->waitq, TASK_NORMAL, 0, &ctx);
 	}
 }
 
@@ -3030,8 +3039,6 @@ static void ioc_pd_free(struct blkg_policy_data *pd)
 		WARN_ON_ONCE(!list_empty(&iocg->surplus_list));
 
 		spin_unlock_irqrestore(&ioc->lock, flags);
-
-		hrtimer_cancel(&iocg->waitq_timer);
 	}
 	free_percpu(iocg->pcpu_stat);
 	kfree(iocg);
-- 
2.31.1

Re: [PATCH -next 3/4] blk-iocost: dispatch all throttled bio in ioc_pd_offline

[Tejun Heo]

On Sat, Dec 17, 2022 at 11:05:26AM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Currently, if cgroup is removed while some bio is still throttled, such
> bio will still wait for timer to dispatch. On the one hand, it
> doesn't make sense to throttle bio while cgroup is removed, on the other
> hand, this behaviour makes it hard to guarantee the exit order for
> iocg(in ioc_pd_free() currently).

Yeah, idk about this. So, now if you're in a cgroup with IO restriction, you
can create a sub-cgroup shove all the IOs in there and then kill the cgroup
and escape control and do so repeatedly.

The refcnting is rather complicated in blkcg world because it gets pulled
from several different directions but the problem you're trying to address
likely should be addressed through refcnting.

Thanks.

-- 
tejun

[PATCH -next 4/4] blk-iocost: guarantee the exit order of iocg

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

pd_offline_fn() is ensured to be called in order(parent after child),
while pd_free_fn() is not. Furthermore, rq_qos_exit() can also be called
before pd_free_fn(). For conclusion, doing cleanups in ioc_pd_free() is
not safe, which can cause many problems. By the way, iocost is the only
policy to do cleanups in pd_free_fn().

Since previous patches guarantee that ioc_pd_offline() will dispatch
throttled bio and prevent new io to be throttled. It's safe to move
cleanup operations for iocg from ioc_pd_free() to ioc_pd_offline().

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-iocost.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index b63ecfdd815c..d634ea73f991 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -3011,21 +3011,6 @@ static void ioc_pd_offline(struct blkg_policy_data *pd)
 
 		iocg_lock(iocg, true, &flags);
 		iocg->online = false;
-		iocg_unlock(iocg, true, &flags);
-
-		hrtimer_cancel(&iocg->waitq_timer);
-		__wake_up(&iocg->waitq, TASK_NORMAL, 0, &ctx);
-	}
-}
-
-static void ioc_pd_free(struct blkg_policy_data *pd)
-{
-	struct ioc_gq *iocg = pd_to_iocg(pd);
-	struct ioc *ioc = iocg->ioc;
-	unsigned long flags;
-
-	if (ioc) {
-		spin_lock_irqsave(&ioc->lock, flags);
 
 		if (!list_empty(&iocg->active_list)) {
 			struct ioc_now now;
@@ -3038,8 +3023,17 @@ static void ioc_pd_free(struct blkg_policy_data *pd)
 		WARN_ON_ONCE(!list_empty(&iocg->walk_list));
 		WARN_ON_ONCE(!list_empty(&iocg->surplus_list));
 
-		spin_unlock_irqrestore(&ioc->lock, flags);
+		iocg_unlock(iocg, true, &flags);
+
+		hrtimer_cancel(&iocg->waitq_timer);
+		__wake_up(&iocg->waitq, TASK_NORMAL, 0, &ctx);
 	}
+}
+
+static void ioc_pd_free(struct blkg_policy_data *pd)
+{
+	struct ioc_gq *iocg = pd_to_iocg(pd);
+
 	free_percpu(iocg->pcpu_stat);
 	kfree(iocg);
 }
-- 
2.31.1