From: Yu Kuai <[email protected]>
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
From: Yu Kuai <[email protected]>
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 <[email protected]>
---
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
From: Yu Kuai <[email protected]>
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 <[email protected]>
---
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
On Sat, Dec 17, 2022 at 11:05:25AM +0800, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> 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
Hi,
?? 2022/12/20 5:28, Tejun Heo д??:
> On Sat, Dec 17, 2022 at 11:05:25AM +0800, Yu Kuai wrote:
>> From: Yu Kuai <[email protected]>
>>
>> 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.
>
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?
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
> .
>