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

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
> .
>