[PATCH -next 0/5] blk-iocost: some random patches to improve iocost

[PATCH -next 0/5] blk-iocost: some random patches to improve iocost

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Yu Kuai (5):
  blk-iocost: disable writeback throttling
  blk-iocost: don't release 'ioc->lock' while updating params
  blk-iocost: prevent configuration update concurrent with io throttling
  blk-iocost: bypass if only one cgroup issues io
  blk-iocost: read 'ioc->params' inside 'ioc->lock' in ioc_timer_fn()

 block/blk-iocost.c | 50 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 11 deletions(-)

-- 
2.31.1

[PATCH -next 1/5] blk-iocost: disable writeback throttling

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Commit b5dc5d4d1f4f ("block,bfq: Disable writeback throttling") disable
wbt for bfq, because different write-throttling heuristics should not
work together.

For the same reason, wbt and iocost should not work together as well,
unless admin really want to do that, dispite that performance is
affected.

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

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 495396425bad..08036476e6fa 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -3264,9 +3264,11 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 		blk_stat_enable_accounting(disk->queue);
 		blk_queue_flag_set(QUEUE_FLAG_RQ_ALLOC_TIME, disk->queue);
 		ioc->enabled = true;
+		wbt_disable_default(disk->queue);
 	} else {
 		blk_queue_flag_clear(QUEUE_FLAG_RQ_ALLOC_TIME, disk->queue);
 		ioc->enabled = false;
+		wbt_enable_default(disk->queue);
 	}
 
 	if (user) {
-- 
2.31.1

Re: [PATCH -next 1/5] blk-iocost: disable writeback throttling

[Tejun Heo]

On Tue, Oct 11, 2022 at 04:35:43PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Commit b5dc5d4d1f4f ("block,bfq: Disable writeback throttling") disable
> wbt for bfq, because different write-throttling heuristics should not
> work together.
> 
> For the same reason, wbt and iocost should not work together as well,
> unless admin really want to do that, dispite that performance is
> affected.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[PATCH -next 2/5] blk-iocost: don't release 'ioc->lock' while updating params

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

ioc_qos_write() and ioc_cost_model_write() are the same:

1) hold lock to read 'ioc->params' to local variable;
2) update params to local variable without lock;
3) hold lock to write local variable to 'ioc->params';

In theroy, if user updates params concurrenty, the params might be lost:

t1: update params a		t2: update params b
spin_lock_irq(&ioc->lock);
memcpy(qos, ioc->params.qos, sizeof(qos))
spin_unlock_irq(&ioc->lock);

qos[a] = xxx;

				spin_lock_irq(&ioc->lock);
				memcpy(qos, ioc->params.qos, sizeof(qos))
				spin_unlock_irq(&ioc->lock);

				qos[b] = xxx;

spin_lock_irq(&ioc->lock);
memcpy(ioc->params.qos, qos, sizeof(qos));
ioc_refresh_params(ioc, true);
spin_unlock_irq(&ioc->lock);

				spin_lock_irq(&ioc->lock);
				// updates of a will be lost
				memcpy(ioc->params.qos, qos, sizeof(qos));
				ioc_refresh_params(ioc, true);
				spin_unlock_irq(&ioc->lock);

Althrough this is not common case, the problem can by fixed easily by
holding the lock through the read, update, write process.

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

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 08036476e6fa..6d36a4bd4382 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -3191,7 +3191,6 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 	memcpy(qos, ioc->params.qos, sizeof(qos));
 	enable = ioc->enabled;
 	user = ioc->user_qos_params;
-	spin_unlock_irq(&ioc->lock);
 
 	while ((p = strsep(&input, " \t\n"))) {
 		substring_t args[MAX_OPT_ARGS];
@@ -3258,8 +3257,6 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 	if (qos[QOS_MIN] > qos[QOS_MAX])
 		goto einval;
 
-	spin_lock_irq(&ioc->lock);
-
 	if (enable) {
 		blk_stat_enable_accounting(disk->queue);
 		blk_queue_flag_set(QUEUE_FLAG_RQ_ALLOC_TIME, disk->queue);
@@ -3284,6 +3281,7 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 	blkdev_put_no_open(bdev);
 	return nbytes;
 einval:
+	spin_unlock_irq(&ioc->lock);
 	ret = -EINVAL;
 err:
 	blkdev_put_no_open(bdev);
@@ -3359,7 +3357,6 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 	spin_lock_irq(&ioc->lock);
 	memcpy(u, ioc->params.i_lcoefs, sizeof(u));
 	user = ioc->user_cost_model;
-	spin_unlock_irq(&ioc->lock);
 
 	while ((p = strsep(&input, " \t\n"))) {
 		substring_t args[MAX_OPT_ARGS];
@@ -3396,7 +3393,6 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 		user = true;
 	}
 
-	spin_lock_irq(&ioc->lock);
 	if (user) {
 		memcpy(ioc->params.i_lcoefs, u, sizeof(u));
 		ioc->user_cost_model = true;
@@ -3410,6 +3406,7 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 	return nbytes;
 
 einval:
+	spin_unlock_irq(&ioc->lock);
 	ret = -EINVAL;
 err:
 	blkdev_put_no_open(bdev);
-- 
2.31.1

Re: [PATCH -next 2/5] blk-iocost: don't release 'ioc->lock' while updating params

[Tejun Heo]

On Tue, Oct 11, 2022 at 04:35:44PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> ioc_qos_write() and ioc_cost_model_write() are the same:
> 
> 1) hold lock to read 'ioc->params' to local variable;
> 2) update params to local variable without lock;
> 3) hold lock to write local variable to 'ioc->params';
> 
> In theroy, if user updates params concurrenty, the params might be lost:
> 
> t1: update params a		t2: update params b
> spin_lock_irq(&ioc->lock);
> memcpy(qos, ioc->params.qos, sizeof(qos))
> spin_unlock_irq(&ioc->lock);
> 
> qos[a] = xxx;
> 
> 				spin_lock_irq(&ioc->lock);
> 				memcpy(qos, ioc->params.qos, sizeof(qos))
> 				spin_unlock_irq(&ioc->lock);
> 
> 				qos[b] = xxx;
> 
> spin_lock_irq(&ioc->lock);
> memcpy(ioc->params.qos, qos, sizeof(qos));
> ioc_refresh_params(ioc, true);
> spin_unlock_irq(&ioc->lock);
> 
> 				spin_lock_irq(&ioc->lock);
> 				// updates of a will be lost
> 				memcpy(ioc->params.qos, qos, sizeof(qos));
> 				ioc_refresh_params(ioc, true);
> 				spin_unlock_irq(&ioc->lock);
> 
> Althrough this is not common case, the problem can by fixed easily by
> holding the lock through the read, update, write process.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[PATCH -next 3/5] blk-iocost: prevent configuration update concurrent with io throttling

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

This won't cause any severe problem currently, however, this doesn't
seems appropriate:

1) 'ioc->params' is read from multiple places without holding
'ioc->lock', unexpected value might be read if writing it concurrently.

2) If configuration is changed while io is throttling, the functionality
might be affected. For example, if module params is updated and cost
becomes smaller, waiting for timer that is caculated under old
configuration is not appropriate.

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

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 6d36a4bd4382..5acc5f13bbd6 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -3187,6 +3187,9 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 		ioc = q_to_ioc(disk->queue);
 	}
 
+	blk_mq_freeze_queue(disk->queue);
+	blk_mq_quiesce_queue(disk->queue);
+
 	spin_lock_irq(&ioc->lock);
 	memcpy(qos, ioc->params.qos, sizeof(qos));
 	enable = ioc->enabled;
@@ -3278,10 +3281,17 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 	ioc_refresh_params(ioc, true);
 	spin_unlock_irq(&ioc->lock);
 
+	blk_mq_unquiesce_queue(disk->queue);
+	blk_mq_unfreeze_queue(disk->queue);
+
 	blkdev_put_no_open(bdev);
 	return nbytes;
 einval:
 	spin_unlock_irq(&ioc->lock);
+
+	blk_mq_unquiesce_queue(disk->queue);
+	blk_mq_unfreeze_queue(disk->queue);
+
 	ret = -EINVAL;
 err:
 	blkdev_put_no_open(bdev);
@@ -3336,6 +3346,7 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 				    size_t nbytes, loff_t off)
 {
 	struct block_device *bdev;
+	struct request_queue *q;
 	struct ioc *ioc;
 	u64 u[NR_I_LCOEFS];
 	bool user;
@@ -3346,14 +3357,18 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 	if (IS_ERR(bdev))
 		return PTR_ERR(bdev);
 
-	ioc = q_to_ioc(bdev_get_queue(bdev));
+	q = bdev_get_queue(bdev);
+	ioc = q_to_ioc(q);
 	if (!ioc) {
 		ret = blk_iocost_init(bdev->bd_disk);
 		if (ret)
 			goto err;
-		ioc = q_to_ioc(bdev_get_queue(bdev));
+		ioc = q_to_ioc(q);
 	}
 
+	blk_mq_freeze_queue(q);
+	blk_mq_quiesce_queue(q);
+
 	spin_lock_irq(&ioc->lock);
 	memcpy(u, ioc->params.i_lcoefs, sizeof(u));
 	user = ioc->user_cost_model;
@@ -3402,11 +3417,18 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 	ioc_refresh_params(ioc, true);
 	spin_unlock_irq(&ioc->lock);
 
+	blk_mq_unquiesce_queue(q);
+	blk_mq_unfreeze_queue(q);
+
 	blkdev_put_no_open(bdev);
 	return nbytes;
 
 einval:
 	spin_unlock_irq(&ioc->lock);
+
+	blk_mq_unquiesce_queue(q);
+	blk_mq_unfreeze_queue(q);
+
 	ret = -EINVAL;
 err:
 	blkdev_put_no_open(bdev);
-- 
2.31.1

Re: [PATCH -next 3/5] blk-iocost: prevent configuration update concurrent with io throttling

[Tejun Heo]

On Tue, Oct 11, 2022 at 04:35:45PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> This won't cause any severe problem currently, however, this doesn't
> seems appropriate:
> 
> 1) 'ioc->params' is read from multiple places without holding
> 'ioc->lock', unexpected value might be read if writing it concurrently.
> 
> 2) If configuration is changed while io is throttling, the functionality
> might be affected. For example, if module params is updated and cost
> becomes smaller, waiting for timer that is caculated under old
> configuration is not appropriate.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[PATCH -next 4/5] blk-iocost: bypass if only one cgroup issues io

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

In this special case, there is no need to throttle io.

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

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 5acc5f13bbd6..32e7e416d67c 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2564,8 +2564,13 @@ static void ioc_rqos_throttle(struct rq_qos *rqos, struct bio *bio)
 	bool use_debt, ioc_locked;
 	unsigned long flags;
 
-	/* bypass IOs if disabled, still initializing, or for root cgroup */
-	if (!ioc->enabled || !iocg || !iocg->level)
+	/*
+	 * bypass IOs if disabled, still initializing, for root cgroup,
+	 * or the cgroup is the only cgroup with io.
+	 */
+	if (!ioc->enabled || !iocg || !iocg->level ||
+	    (iocg->hweight_inuse == WEIGHT_ONE &&
+	     atomic_read(&ioc->hweight_gen) == iocg->hweight_gen))
 		return;
 
 	/* calculate the absolute vtime cost */
-- 
2.31.1

Re: [PATCH -next 4/5] blk-iocost: bypass if only one cgroup issues io

[Tejun Heo]

On Tue, Oct 11, 2022 at 04:35:46PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> In this special case, there is no need to throttle io.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  block/blk-iocost.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index 5acc5f13bbd6..32e7e416d67c 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c
> @@ -2564,8 +2564,13 @@ static void ioc_rqos_throttle(struct rq_qos *rqos, struct bio *bio)
>  	bool use_debt, ioc_locked;
>  	unsigned long flags;
>  
> -	/* bypass IOs if disabled, still initializing, or for root cgroup */
> -	if (!ioc->enabled || !iocg || !iocg->level)
> +	/*
> +	 * bypass IOs if disabled, still initializing, for root cgroup,
> +	 * or the cgroup is the only cgroup with io.
> +	 */
> +	if (!ioc->enabled || !iocg || !iocg->level ||
> +	    (iocg->hweight_inuse == WEIGHT_ONE &&
> +	     atomic_read(&ioc->hweight_gen) == iocg->hweight_gen))

I'm not sure about this one. Bypassing here means that we lose track of how
much IO it's issuing which can affect future throttling decisions, right?

Thanks.

-- 
tejun

Re: [PATCH -next 4/5] blk-iocost: bypass if only one cgroup issues io

[Yu Kuai]

Hi, Tejun!

在 2022/10/12 1:02, Tejun Heo 写道:
> On Tue, Oct 11, 2022 at 04:35:46PM +0800, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> In this special case, there is no need to throttle io.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   block/blk-iocost.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
>> index 5acc5f13bbd6..32e7e416d67c 100644
>> --- a/block/blk-iocost.c
>> +++ b/block/blk-iocost.c
>> @@ -2564,8 +2564,13 @@ static void ioc_rqos_throttle(struct rq_qos *rqos, struct bio *bio)
>>   	bool use_debt, ioc_locked;
>>   	unsigned long flags;
>>   
>> -	/* bypass IOs if disabled, still initializing, or for root cgroup */
>> -	if (!ioc->enabled || !iocg || !iocg->level)
>> +	/*
>> +	 * bypass IOs if disabled, still initializing, for root cgroup,
>> +	 * or the cgroup is the only cgroup with io.
>> +	 */
>> +	if (!ioc->enabled || !iocg || !iocg->level ||
>> +	    (iocg->hweight_inuse == WEIGHT_ONE &&
>> +	     atomic_read(&ioc->hweight_gen) == iocg->hweight_gen))
> 
> I'm not sure about this one. Bypassing here means that we lose track of how
> much IO it's issuing which can affect future throttling decisions, right?

Yes, you're right, this patch doesn't look good in this case.

The reason why I tried to do this is because during test, I found that
io performance is affected when I only issue io from one cgroup（only
happened in some environment with default configuration）, and I found
out that each io is throttled for some time before dispatching.

Perhaps a suitable configuration can avoid this problem.

Thanks,
Kuai

> 
> Thanks.
> 

Re: [PATCH -next 4/5] blk-iocost: bypass if only one cgroup issues io

[Tejun Heo]

On Wed, Oct 12, 2022 at 09:33:46AM +0800, Yu Kuai wrote:
> Perhaps a suitable configuration can avoid this problem.

Yeah, iocost is a throttling controller. The default parameters try to be
adaptive but it really needs benchmarked parameters to work properly.

Thanks.

-- 
tejun

[PATCH -next 5/5] blk-iocost: read 'ioc->params' inside 'ioc->lock' in ioc_timer_fn()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

'ioc->params' is updated in ioc_refresh_params(), which is proteced by
'ioc->lock', however, ioc_timer_fn() read params outside the lock.

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

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 32e7e416d67c..acb10ba49da9 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2203,8 +2203,8 @@ static void ioc_timer_fn(struct timer_list *timer)
 	LIST_HEAD(surpluses);
 	int nr_debtors, nr_shortages = 0, nr_lagging = 0;
 	u64 usage_us_sum = 0;
-	u32 ppm_rthr = MILLION - ioc->params.qos[QOS_RPPM];
-	u32 ppm_wthr = MILLION - ioc->params.qos[QOS_WPPM];
+	u32 ppm_rthr;
+	u32 ppm_wthr;
 	u32 missed_ppm[2], rq_wait_pct;
 	u64 period_vtime;
 	int prev_busy_level;
@@ -2215,6 +2215,8 @@ static void ioc_timer_fn(struct timer_list *timer)
 	/* take care of active iocgs */
 	spin_lock_irq(&ioc->lock);
 
+	ppm_rthr = MILLION - ioc->params.qos[QOS_RPPM];
+	ppm_wthr = MILLION - ioc->params.qos[QOS_WPPM];
 	ioc_now(ioc, &now);
 
 	period_vtime = now.vnow - ioc->period_at_vtime;
-- 
2.31.1

Re: [PATCH -next 5/5] blk-iocost: read 'ioc->params' inside 'ioc->lock' in ioc_timer_fn()

[Tejun Heo]

On Tue, Oct 11, 2022 at 04:35:47PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> 'ioc->params' is updated in ioc_refresh_params(), which is proteced by
> 'ioc->lock', however, ioc_timer_fn() read params outside the lock.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun