[PATCH v7 0/9] bugfix and cleanup for blk-throttle

[PATCH v7 0/9] bugfix and cleanup for blk-throttle

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Changes in v7:
 - add patch 5 to improve handling of re-entered bio for bps limit
 - as suggested by Tejun, add some comments
 - sdd some Acked tag by Tejun
Changes in v6:
 - rename parameter in patch 3
 - add comments and reviewed tag for patch 4
Changes in v5:
 - add comments in patch 4
 - clear bytes/io_skipped in throtl_start_new_slice_with_credit() in
 patch 4
 - and cleanup patches 5-8
Changes in v4:
 - add reviewed-by tag for patch 1
 - add patch 2,3
 - use a different way to fix io hung in patch 4
Changes in v3:
 - fix a check in patch 1
 - fix link err in patch 2 on 32-bit platform
 - handle overflow in patch 2
Changes in v2:
 - use a new solution suggested by Ming
 - change the title of patch 1
 - add patch 2

Patch 1 fix that blk-throttle can't work if multiple bios are throttle.
Patch 2 fix overflow while calculating wait time.
Patch 3,4 fix io hung due to configuration updates.
Patch 5 improve handling of re-entered bio for bps limit.
Patch 6-9 are cleanup patches, there are no functional changes, just
some places that I think can be optimized during code review.

Previous version:
v1: https://lore.kernel.org/all/20220517134909.2910251-1-yukuai3@huawei.com/
v2: https://lore.kernel.org/all/20220518072751.1188163-1-yukuai3@huawei.com/
v3: https://lore.kernel.org/all/20220519085811.879097-1-yukuai3@huawei.com/
v4: https://lore.kernel.org/all/20220523082633.2324980-1-yukuai3@huawei.com/
v5: https://lore.kernel.org/all/20220528064330.3471000-1-yukuai3@huawei.com/
v6: https://lore.kernel.org/all/20220701093441.885741-1-yukuai1@huaweicloud.com/

Yu Kuai (9):
  blk-throttle: fix that io throttle can only work for single bio
  blk-throttle: prevent overflow while calculating wait time
  blk-throttle: factor out code to calculate ios/bytes_allowed
  blk-throttle: fix io hung due to configuration updates
  blk-throttle: improve handling of re-entered bio for bps limit
  blk-throttle: use 'READ/WRITE' instead of '0/1'
  blk-throttle: calling throtl_dequeue/enqueue_tg in pairs
  blk-throttle: cleanup tg_update_disptime()
  blk-throttle: clean up flag 'THROTL_TG_PENDING'

 block/blk-throttle.c | 175 ++++++++++++++++++++++++++++++-------------
 block/blk-throttle.h |  20 ++++-
 2 files changed, 139 insertions(+), 56 deletions(-)

-- 
2.31.1

[PATCH v7 0/9] bugfix and cleanup for blk-throttle

[Yu Kuai]

From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Changes in v7:
 - add patch 5 to improve handling of re-entered bio for bps limit
 - as suggested by Tejun, add some comments
 - sdd some Acked tag by Tejun
Changes in v6:
 - rename parameter in patch 3
 - add comments and reviewed tag for patch 4
Changes in v5:
 - add comments in patch 4
 - clear bytes/io_skipped in throtl_start_new_slice_with_credit() in
 patch 4
 - and cleanup patches 5-8
Changes in v4:
 - add reviewed-by tag for patch 1
 - add patch 2,3
 - use a different way to fix io hung in patch 4
Changes in v3:
 - fix a check in patch 1
 - fix link err in patch 2 on 32-bit platform
 - handle overflow in patch 2
Changes in v2:
 - use a new solution suggested by Ming
 - change the title of patch 1
 - add patch 2

Patch 1 fix that blk-throttle can't work if multiple bios are throttle.
Patch 2 fix overflow while calculating wait time.
Patch 3,4 fix io hung due to configuration updates.
Patch 5 improve handling of re-entered bio for bps limit.
Patch 6-9 are cleanup patches, there are no functional changes, just
some places that I think can be optimized during code review.

Previous version:
v1: https://lore.kernel.org/all/20220517134909.2910251-1-yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org/
v2: https://lore.kernel.org/all/20220518072751.1188163-1-yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org/
v3: https://lore.kernel.org/all/20220519085811.879097-1-yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org/
v4: https://lore.kernel.org/all/20220523082633.2324980-1-yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org/
v5: https://lore.kernel.org/all/20220528064330.3471000-1-yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org/
v6: https://lore.kernel.org/all/20220701093441.885741-1-yukuai1-XF6JlduFytWkHkcT6e4Xnw@public.gmane.org/

Yu Kuai (9):
  blk-throttle: fix that io throttle can only work for single bio
  blk-throttle: prevent overflow while calculating wait time
  blk-throttle: factor out code to calculate ios/bytes_allowed
  blk-throttle: fix io hung due to configuration updates
  blk-throttle: improve handling of re-entered bio for bps limit
  blk-throttle: use 'READ/WRITE' instead of '0/1'
  blk-throttle: calling throtl_dequeue/enqueue_tg in pairs
  blk-throttle: cleanup tg_update_disptime()
  blk-throttle: clean up flag 'THROTL_TG_PENDING'

 block/blk-throttle.c | 175 ++++++++++++++++++++++++++++++-------------
 block/blk-throttle.h |  20 ++++-
 2 files changed, 139 insertions(+), 56 deletions(-)

-- 
2.31.1

[PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Test scripts:
cd /sys/fs/cgroup/blkio/
echo "8:0 1024" > blkio.throttle.write_bps_device
echo $$ > cgroup.procs
dd if=/dev/zero of=/dev/sda bs=10k count=1 oflag=direct &
dd if=/dev/zero of=/dev/sda bs=10k count=1 oflag=direct &

Test result:
10240 bytes (10 kB, 10 KiB) copied, 10.0134 s, 1.0 kB/s
10240 bytes (10 kB, 10 KiB) copied, 10.0135 s, 1.0 kB/s

The problem is that the second bio is finished after 10s instead of 20s.

Root cause:
1) second bio will be flaged:

__blk_throtl_bio
 while (true) {
  ...
  if (sq->nr_queued[rw]) -> some bio is throttled already
   break
 };
 bio_set_flag(bio, BIO_THROTTLED); -> flag the bio

2) flaged bio will be dispatched without waiting:

throtl_dispatch_tg
 tg_may_dispatch
  tg_with_in_bps_limit
   if (bps_limit == U64_MAX || bio_flagged(bio, BIO_THROTTLED))
    *wait = 0; -> wait time is zero
    return true;

commit 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
support to count splited bios for iops limit, thus it adds flaged bio
checking in tg_with_in_bps_limit() so that splited bios will only count
once for bps limit, however, it introduce a new problem that io throttle
won't work if multiple bios are throttled.

In order to fix the problem, at first, don't skip flaged bio in
tg_with_in_bps_limit(), however, this will break that splited bios should
only count once for bps limit. And this patch tries to avoid
over-accounting by decrementing it first in __blk_throtl_bio(), and
then counting it again while dispatching it.

Fixes: 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-throttle.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 9f5fe62afff9..2957e2c643f4 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -811,7 +811,7 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
 	unsigned int bio_size = throtl_bio_data_size(bio);
 
 	/* no need to throttle if this bio's bytes have been accounted */
-	if (bps_limit == U64_MAX || bio_flagged(bio, BIO_THROTTLED)) {
+	if (bps_limit == U64_MAX) {
 		if (wait)
 			*wait = 0;
 		return true;
@@ -921,11 +921,8 @@ static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio)
 	unsigned int bio_size = throtl_bio_data_size(bio);
 
 	/* Charge the bio to the group */
-	if (!bio_flagged(bio, BIO_THROTTLED)) {
-		tg->bytes_disp[rw] += bio_size;
-		tg->last_bytes_disp[rw] += bio_size;
-	}
-
+	tg->bytes_disp[rw] += bio_size;
+	tg->last_bytes_disp[rw] += bio_size;
 	tg->io_disp[rw]++;
 	tg->last_io_disp[rw]++;
 
@@ -2121,6 +2118,23 @@ bool __blk_throtl_bio(struct bio *bio)
 			tg->last_low_overflow_time[rw] = jiffies;
 		throtl_downgrade_check(tg);
 		throtl_upgrade_check(tg);
+
+		/*
+		 * Splited bios can be re-entered because iops limit should be
+		 * counted again, however, bps limit should not. Since bps limit
+		 * will be counted again while dispatching it, compensate the
+		 * over-accounting here. Noted that compensation can fail if
+		 * new slice is started.
+		 */
+		if (bio_flagged(bio, BIO_THROTTLED)) {
+			unsigned int bio_size = throtl_bio_data_size(bio);
+
+			if (tg->bytes_disp[rw] >= bio_size)
+				tg->bytes_disp[rw] -= bio_size;
+			if (tg->last_bytes_disp[rw] >= bio_size)
+				tg->last_bytes_disp[rw] -= bio_size;
+		}
+
 		/* throtl is FIFO - if bios are already queued, should queue */
 		if (sq->nr_queued[rw])
 			break;
-- 
2.31.1

Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Tejun Heo]

On Tue, Aug 02, 2022 at 10:04:07PM +0800, Yu Kuai wrote:
...
> commit 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
> support to count splited bios for iops limit, thus it adds flaged bio
                                                             ^
                                                             flagged

> checking in tg_with_in_bps_limit() so that splited bios will only count
                                             ^
                                             split

> once for bps limit, however, it introduce a new problem that io throttle
> won't work if multiple bios are throttled.
> 
> In order to fix the problem, at first, don't skip flaged bio in
> tg_with_in_bps_limit(), however, this will break that splited bios should
> only count once for bps limit. And this patch tries to avoid
> over-accounting by decrementing it first in __blk_throtl_bio(), and
> then counting it again while dispatching it.
> 
> Fixes: 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> Reviewed-by: Ming Lei <ming.lei@redhat.com>

Please cc stable w/ version tag.

> ---
>  block/blk-throttle.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 9f5fe62afff9..2957e2c643f4 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -811,7 +811,7 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
>  	unsigned int bio_size = throtl_bio_data_size(bio);
>  
>  	/* no need to throttle if this bio's bytes have been accounted */
> -	if (bps_limit == U64_MAX || bio_flagged(bio, BIO_THROTTLED)) {
> +	if (bps_limit == U64_MAX) {
>  		if (wait)
>  			*wait = 0;
>  		return true;
> @@ -921,11 +921,8 @@ static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio)
>  	unsigned int bio_size = throtl_bio_data_size(bio);
>  
>  	/* Charge the bio to the group */
> -	if (!bio_flagged(bio, BIO_THROTTLED)) {
> -		tg->bytes_disp[rw] += bio_size;
> -		tg->last_bytes_disp[rw] += bio_size;
> -	}
> -
> +	tg->bytes_disp[rw] += bio_size;
> +	tg->last_bytes_disp[rw] += bio_size;
>  	tg->io_disp[rw]++;
>  	tg->last_io_disp[rw]++;
>  
> @@ -2121,6 +2118,23 @@ bool __blk_throtl_bio(struct bio *bio)
>  			tg->last_low_overflow_time[rw] = jiffies;
>  		throtl_downgrade_check(tg);
>  		throtl_upgrade_check(tg);
> +
> +		/*
> +		 * Splited bios can be re-entered because iops limit should be
                   ^                ^^^^^^^^^^^^^
                   Split            re-enter

> +		 * counted again, however, bps limit should not. Since bps limit
> +		 * will be counted again while dispatching it, compensate the
> +		 * over-accounting here. Noted that compensation can fail if
> +		 * new slice is started.

I can't really follow the comment. Please improve the explanation.

> +		 */
> +		if (bio_flagged(bio, BIO_THROTTLED)) {
> +			unsigned int bio_size = throtl_bio_data_size(bio);
> +
> +			if (tg->bytes_disp[rw] >= bio_size)
> +				tg->bytes_disp[rw] -= bio_size;
> +			if (tg->last_bytes_disp[rw] >= bio_size)
> +				tg->last_bytes_disp[rw] -= bio_size;
> +		}

So, as a fix for the immediate problem, I guess this might do but this feels
really fragile. How can we be certain that re-entering only happens because
of splitting? What if future core development changes that? It seems to be
solving the problem in the wrong place. Shouldn't we flag the bio indicating
that it's split when we're splitting the bio so that we only limit them for
iops in the first place?

Thanks.

-- 
tejun

Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Yu Kuai]

Hi, Tejun

在 2022/08/17 3:37, Tejun Heo 写道:
> On Tue, Aug 02, 2022 at 10:04:07PM +0800, Yu Kuai wrote:
> ...
>> commit 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
>> support to count splited bios for iops limit, thus it adds flaged bio
>                                                               ^
>                                                               flagged
> 
>> checking in tg_with_in_bps_limit() so that splited bios will only count
>                                               ^
>                                               split
> 
>> once for bps limit, however, it introduce a new problem that io throttle
>> won't work if multiple bios are throttled.
>>
>> In order to fix the problem, at first, don't skip flaged bio in
>> tg_with_in_bps_limit(), however, this will break that splited bios should
>> only count once for bps limit. And this patch tries to avoid
>> over-accounting by decrementing it first in __blk_throtl_bio(), and
>> then counting it again while dispatching it.
>>
>> Fixes: 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> Reviewed-by: Ming Lei <ming.lei@redhat.com>
> 
> Please cc stable w/ version tag.
> 
>> ---
>>   block/blk-throttle.c | 26 ++++++++++++++++++++------
>>   1 file changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
>> index 9f5fe62afff9..2957e2c643f4 100644
>> --- a/block/blk-throttle.c
>> +++ b/block/blk-throttle.c
>> @@ -811,7 +811,7 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
>>   	unsigned int bio_size = throtl_bio_data_size(bio);
>>   
>>   	/* no need to throttle if this bio's bytes have been accounted */
>> -	if (bps_limit == U64_MAX || bio_flagged(bio, BIO_THROTTLED)) {
>> +	if (bps_limit == U64_MAX) {
>>   		if (wait)
>>   			*wait = 0;
>>   		return true;
>> @@ -921,11 +921,8 @@ static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio)
>>   	unsigned int bio_size = throtl_bio_data_size(bio);
>>   
>>   	/* Charge the bio to the group */
>> -	if (!bio_flagged(bio, BIO_THROTTLED)) {
>> -		tg->bytes_disp[rw] += bio_size;
>> -		tg->last_bytes_disp[rw] += bio_size;
>> -	}
>> -
>> +	tg->bytes_disp[rw] += bio_size;
>> +	tg->last_bytes_disp[rw] += bio_size;
>>   	tg->io_disp[rw]++;
>>   	tg->last_io_disp[rw]++;
>>   
>> @@ -2121,6 +2118,23 @@ bool __blk_throtl_bio(struct bio *bio)
>>   			tg->last_low_overflow_time[rw] = jiffies;
>>   		throtl_downgrade_check(tg);
>>   		throtl_upgrade_check(tg);
>> +
>> +		/*
>> +		 * Splited bios can be re-entered because iops limit should be
>                     ^                ^^^^^^^^^^^^^
>                     Split            re-enter
> 
>> +		 * counted again, however, bps limit should not. Since bps limit
>> +		 * will be counted again while dispatching it, compensate the
>> +		 * over-accounting here. Noted that compensation can fail if
>> +		 * new slice is started.
> 
> I can't really follow the comment. Please improve the explanation.
> 
>> +		 */
>> +		if (bio_flagged(bio, BIO_THROTTLED)) {
>> +			unsigned int bio_size = throtl_bio_data_size(bio);
>> +
>> +			if (tg->bytes_disp[rw] >= bio_size)
>> +				tg->bytes_disp[rw] -= bio_size;
>> +			if (tg->last_bytes_disp[rw] >= bio_size)
>> +				tg->last_bytes_disp[rw] -= bio_size;
>> +		}
> 
> So, as a fix for the immediate problem, I guess this might do but this feels
> really fragile. How can we be certain that re-entering only happens because
> of splitting? What if future core development changes that? It seems to be
> solving the problem in the wrong place. Shouldn't we flag the bio indicating
> that it's split when we're splitting the bio so that we only limit them for
> iops in the first place?
> 
Splited bio is tracked in __bio_clone:

if (bio_flagged(bio_src, BIO_THROTTLED))
	bio_set_flag(bio, BIO_THROTTLED);

And currenty, the iops limit and bps limit are treated differently,
however there are only one flag 'BIO_THROTTLED' and they can't be
distinguished.

Perhaps I can use two flags, for example BIO_IOPS_THROTTLED and
BIO_BPS_THROTTLED, this way only iops limit can be handled and bps
limit can be skipped for splited bio.

What do you think?

Thanks,
Kuai
> Thanks.
> 

Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Yu Kuai]

Hi, Tejun

ÔÚ 2022/08/17 3:37, Tejun Heo дµÀ:
> On Tue, Aug 02, 2022 at 10:04:07PM +0800, Yu Kuai wrote:
> ...
>> commit 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
>> support to count splited bios for iops limit, thus it adds flaged bio
>                                                               ^
>                                                               flagged
> 
>> checking in tg_with_in_bps_limit() so that splited bios will only count
>                                               ^
>                                               split
> 
>> once for bps limit, however, it introduce a new problem that io throttle
>> won't work if multiple bios are throttled.
>>
>> In order to fix the problem, at first, don't skip flaged bio in
>> tg_with_in_bps_limit(), however, this will break that splited bios should
>> only count once for bps limit. And this patch tries to avoid
>> over-accounting by decrementing it first in __blk_throtl_bio(), and
>> then counting it again while dispatching it.
>>
>> Fixes: 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
>> Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> Reviewed-by: Ming Lei <ming.lei-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Please cc stable w/ version tag.
> 
>> ---
>>   block/blk-throttle.c | 26 ++++++++++++++++++++------
>>   1 file changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
>> index 9f5fe62afff9..2957e2c643f4 100644
>> --- a/block/blk-throttle.c
>> +++ b/block/blk-throttle.c
>> @@ -811,7 +811,7 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
>>   	unsigned int bio_size = throtl_bio_data_size(bio);
>>   
>>   	/* no need to throttle if this bio's bytes have been accounted */
>> -	if (bps_limit == U64_MAX || bio_flagged(bio, BIO_THROTTLED)) {
>> +	if (bps_limit == U64_MAX) {
>>   		if (wait)
>>   			*wait = 0;
>>   		return true;
>> @@ -921,11 +921,8 @@ static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio)
>>   	unsigned int bio_size = throtl_bio_data_size(bio);
>>   
>>   	/* Charge the bio to the group */
>> -	if (!bio_flagged(bio, BIO_THROTTLED)) {
>> -		tg->bytes_disp[rw] += bio_size;
>> -		tg->last_bytes_disp[rw] += bio_size;
>> -	}
>> -
>> +	tg->bytes_disp[rw] += bio_size;
>> +	tg->last_bytes_disp[rw] += bio_size;
>>   	tg->io_disp[rw]++;
>>   	tg->last_io_disp[rw]++;
>>   
>> @@ -2121,6 +2118,23 @@ bool __blk_throtl_bio(struct bio *bio)
>>   			tg->last_low_overflow_time[rw] = jiffies;
>>   		throtl_downgrade_check(tg);
>>   		throtl_upgrade_check(tg);
>> +
>> +		/*
>> +		 * Splited bios can be re-entered because iops limit should be
>                     ^                ^^^^^^^^^^^^^
>                     Split            re-enter
> 
>> +		 * counted again, however, bps limit should not. Since bps limit
>> +		 * will be counted again while dispatching it, compensate the
>> +		 * over-accounting here. Noted that compensation can fail if
>> +		 * new slice is started.
> 
> I can't really follow the comment. Please improve the explanation.
> 
>> +		 */
>> +		if (bio_flagged(bio, BIO_THROTTLED)) {
>> +			unsigned int bio_size = throtl_bio_data_size(bio);
>> +
>> +			if (tg->bytes_disp[rw] >= bio_size)
>> +				tg->bytes_disp[rw] -= bio_size;
>> +			if (tg->last_bytes_disp[rw] >= bio_size)
>> +				tg->last_bytes_disp[rw] -= bio_size;
>> +		}
> 
> So, as a fix for the immediate problem, I guess this might do but this feels
> really fragile. How can we be certain that re-entering only happens because
> of splitting? What if future core development changes that? It seems to be
> solving the problem in the wrong place. Shouldn't we flag the bio indicating
> that it's split when we're splitting the bio so that we only limit them for
> iops in the first place?
> 
Splited bio is tracked in __bio_clone:

if (bio_flagged(bio_src, BIO_THROTTLED))
	bio_set_flag(bio, BIO_THROTTLED);

And currenty, the iops limit and bps limit are treated differently,
however there are only one flag 'BIO_THROTTLED' and they can't be
distinguished.

Perhaps I can use two flags, for example BIO_IOPS_THROTTLED and
BIO_BPS_THROTTLED, this way only iops limit can be handled and bps
limit can be skipped for splited bio.

What do you think?

Thanks,
Kuai
> Thanks.
> 

Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Tejun Heo]

Hello,

On Wed, Aug 17, 2022 at 09:13:38AM +0800, Yu Kuai wrote:
> > So, as a fix for the immediate problem, I guess this might do but this feels
> > really fragile. How can we be certain that re-entering only happens because
> > of splitting? What if future core development changes that? It seems to be
> > solving the problem in the wrong place. Shouldn't we flag the bio indicating
> > that it's split when we're splitting the bio so that we only limit them for
> > iops in the first place?
>
> Splited bio is tracked in __bio_clone:

As the word is used in commit messages and comments, the past perfect form
of the verb "split" is "split". It looks like "splitted" is used in rare
cases but dictionary says it's an archaic form.

> if (bio_flagged(bio_src, BIO_THROTTLED))
> 	bio_set_flag(bio, BIO_THROTTLED);
> 
> And currenty, the iops limit and bps limit are treated differently,
> however there are only one flag 'BIO_THROTTLED' and they can't be
> distinguished.
> 
> Perhaps I can use two flags, for example BIO_IOPS_THROTTLED and
> BIO_BPS_THROTTLED, this way only iops limit can be handled and bps
> limit can be skipped for splited bio.
> 
> What do you think?

I think the code would be a lot more intuitive and less fragile if we used
two flags but the bits in the bi_flags field are a scarce resource
unfortunately. Even then, I think the right thing to do here is using two
flags.

Thanks.

-- 
tejun

Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Yu Kuai]

Hi, Tejun!

在 2022/08/18 1:50, Tejun Heo 写道:
> Hello,
> 
> On Wed, Aug 17, 2022 at 09:13:38AM +0800, Yu Kuai wrote:
>>> So, as a fix for the immediate problem, I guess this might do but this feels
>>> really fragile. How can we be certain that re-entering only happens because
>>> of splitting? What if future core development changes that? It seems to be
>>> solving the problem in the wrong place. Shouldn't we flag the bio indicating
>>> that it's split when we're splitting the bio so that we only limit them for
>>> iops in the first place?
>>
>> Splited bio is tracked in __bio_clone:
> 
> As the word is used in commit messages and comments, the past perfect form
> of the verb "split" is "split". It looks like "splitted" is used in rare
> cases but dictionary says it's an archaic form.

Ok, thanks for pointing it out, I'll change that in next iteration.
> 
>> if (bio_flagged(bio_src, BIO_THROTTLED))
>> 	bio_set_flag(bio, BIO_THROTTLED);
>>
>> And currenty, the iops limit and bps limit are treated differently,
>> however there are only one flag 'BIO_THROTTLED' and they can't be
>> distinguished.
>>
>> Perhaps I can use two flags, for example BIO_IOPS_THROTTLED and
>> BIO_BPS_THROTTLED, this way only iops limit can be handled and bps
>> limit can be skipped for splited bio.
>>
>> What do you think?
> 
> I think the code would be a lot more intuitive and less fragile if we used
> two flags but the bits in the bi_flags field are a scarce resource
> unfortunately. Even then, I think the right thing to do here is using two
> flags.

Yes, the field 'bio->bi_flags' is unsigned short, and there are only two
bits left. I'll use the new sulution which will acquire a new bit.

Thanks,
Kuai
> 
> Thanks.
> 

Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Yu Kuai]

Hi, Tejun!

ÔÚ 2022/08/18 1:50, Tejun Heo дµÀ:
> Hello,
> 
> On Wed, Aug 17, 2022 at 09:13:38AM +0800, Yu Kuai wrote:
>>> So, as a fix for the immediate problem, I guess this might do but this feels
>>> really fragile. How can we be certain that re-entering only happens because
>>> of splitting? What if future core development changes that? It seems to be
>>> solving the problem in the wrong place. Shouldn't we flag the bio indicating
>>> that it's split when we're splitting the bio so that we only limit them for
>>> iops in the first place?
>>
>> Splited bio is tracked in __bio_clone:
> 
> As the word is used in commit messages and comments, the past perfect form
> of the verb "split" is "split". It looks like "splitted" is used in rare
> cases but dictionary says it's an archaic form.

Ok, thanks for pointing it out, I'll change that in next iteration.
> 
>> if (bio_flagged(bio_src, BIO_THROTTLED))
>> 	bio_set_flag(bio, BIO_THROTTLED);
>>
>> And currenty, the iops limit and bps limit are treated differently,
>> however there are only one flag 'BIO_THROTTLED' and they can't be
>> distinguished.
>>
>> Perhaps I can use two flags, for example BIO_IOPS_THROTTLED and
>> BIO_BPS_THROTTLED, this way only iops limit can be handled and bps
>> limit can be skipped for splited bio.
>>
>> What do you think?
> 
> I think the code would be a lot more intuitive and less fragile if we used
> two flags but the bits in the bi_flags field are a scarce resource
> unfortunately. Even then, I think the right thing to do here is using two
> flags.

Yes, the field 'bio->bi_flags' is unsigned short, and there are only two
bits left. I'll use the new sulution which will acquire a new bit.

Thanks,
Kuai
> 
> Thanks.
> 

Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Yu Kuai]

Hi,

在 2022/08/18 9:23, Yu Kuai 写道:
> Hi, Tejun!
> 
> 在 2022/08/18 1:50, Tejun Heo 写道:
>> Hello,
>>
>> On Wed, Aug 17, 2022 at 09:13:38AM +0800, Yu Kuai wrote:
>>>> So, as a fix for the immediate problem, I guess this might do but 
>>>> this feels
>>>> really fragile. How can we be certain that re-entering only happens 
>>>> because
>>>> of splitting? What if future core development changes that? It seems 
>>>> to be
>>>> solving the problem in the wrong place. Shouldn't we flag the bio 
>>>> indicating
>>>> that it's split when we're splitting the bio so that we only limit 
>>>> them for
>>>> iops in the first place?
>>>
>>> Splited bio is tracked in __bio_clone:
>>
>> As the word is used in commit messages and comments, the past perfect 
>> form
>> of the verb "split" is "split". It looks like "splitted" is used in rare
>> cases but dictionary says it's an archaic form.
> 
> Ok, thanks for pointing it out, I'll change that in next iteration.
>>
>>> if (bio_flagged(bio_src, BIO_THROTTLED))
>>>     bio_set_flag(bio, BIO_THROTTLED);

While implementing the new method, I found that there seems to be a
misunderstanding here, the code seems to try to add flag to split bio
so that it won't be throttled again for bps limit, however:

1) for blk throttle, split bio is issued directly and will never be
throttled again, while orignal bio will go through throttle path again.
2) if cloned bio is directed to a new disk, the flag is cleared anyway.
>>>
>>> And currenty, the iops limit and bps limit are treated differently,
>>> however there are only one flag 'BIO_THROTTLED' and they can't be
>>> distinguished.
>>>
>>> Perhaps I can use two flags, for example BIO_IOPS_THROTTLED and
>>> BIO_BPS_THROTTLED, this way only iops limit can be handled and bps
>>> limit can be skipped for splited bio.
>>>
>>> What do you think?
>>
>> I think the code would be a lot more intuitive and less fragile if we 
>> used
>> two flags but the bits in the bi_flags field are a scarce resource
>> unfortunately. Even then, I think the right thing to do here is using two
>> flags.
> 
> Yes, the field 'bio->bi_flags' is unsigned short, and there are only two
> bits left. I'll use the new sulution which will acquire a new bit.
> 
> Thanks,
> Kuai
>>
>> Thanks.
>>
> 
> .
> 

Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Yu Kuai]

Hi,

在 2022/08/18 9:23, Yu Kuai 写道:
> Hi, Tejun!
> 
> 在 2022/08/18 1:50, Tejun Heo 写道:
>> Hello,
>>
>> On Wed, Aug 17, 2022 at 09:13:38AM +0800, Yu Kuai wrote:
>>>> So, as a fix for the immediate problem, I guess this might do but 
>>>> this feels
>>>> really fragile. How can we be certain that re-entering only happens 
>>>> because
>>>> of splitting? What if future core development changes that? It seems 
>>>> to be
>>>> solving the problem in the wrong place. Shouldn't we flag the bio 
>>>> indicating
>>>> that it's split when we're splitting the bio so that we only limit 
>>>> them for
>>>> iops in the first place?
>>>
>>> Splited bio is tracked in __bio_clone:
>>
>> As the word is used in commit messages and comments, the past perfect 
>> form
>> of the verb "split" is "split". It looks like "splitted" is used in rare
>> cases but dictionary says it's an archaic form.
> 
> Ok, thanks for pointing it out, I'll change that in next iteration.
>>
>>> if (bio_flagged(bio_src, BIO_THROTTLED))
>>>     bio_set_flag(bio, BIO_THROTTLED);

While implementing the new method, I found that there seems to be a
misunderstanding here, the code seems to try to add flag to split bio
so that it won't be throttled again for bps limit, however:

1) for blk throttle, split bio is issued directly and will never be
throttled again, while orignal bio will go through throttle path again.
2) if cloned bio is directed to a new disk, the flag is cleared anyway.
>>>
>>> And currenty, the iops limit and bps limit are treated differently,
>>> however there are only one flag 'BIO_THROTTLED' and they can't be
>>> distinguished.
>>>
>>> Perhaps I can use two flags, for example BIO_IOPS_THROTTLED and
>>> BIO_BPS_THROTTLED, this way only iops limit can be handled and bps
>>> limit can be skipped for splited bio.
>>>
>>> What do you think?
>>
>> I think the code would be a lot more intuitive and less fragile if we 
>> used
>> two flags but the bits in the bi_flags field are a scarce resource
>> unfortunately. Even then, I think the right thing to do here is using two
>> flags.
> 
> Yes, the field 'bio->bi_flags' is unsigned short, and there are only two
> bits left. I'll use the new sulution which will acquire a new bit.
> 
> Thanks,
> Kuai
>>
>> Thanks.
>>
> 
> .
> 

Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Tejun Heo]

Hello,

On Mon, Aug 22, 2022 at 11:06:44AM +0800, Yu Kuai wrote:
> While implementing the new method, I found that there seems to be a
> misunderstanding here, the code seems to try to add flag to split bio
> so that it won't be throttled again for bps limit, however:
> 
> 1) for blk throttle, split bio is issued directly and will never be
> throttled again, while orignal bio will go through throttle path again.
> 2) if cloned bio is directed to a new disk, the flag is cleared anyway.

Doesn't that make the current code correct then? But you were seeing
incorrect behaviors, right?

Thanks.

-- 
tejun

Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Tejun Heo]

Hello,

On Mon, Aug 22, 2022 at 11:06:44AM +0800, Yu Kuai wrote:
> While implementing the new method, I found that there seems to be a
> misunderstanding here, the code seems to try to add flag to split bio
> so that it won't be throttled again for bps limit, however:
> 
> 1) for blk throttle, split bio is issued directly and will never be
> throttled again, while orignal bio will go through throttle path again.
> 2) if cloned bio is directed to a new disk, the flag is cleared anyway.

Doesn't that make the current code correct then? But you were seeing
incorrect behaviors, right?

Thanks.

-- 
tejun

Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Yu Kuai]

Hi, Tejun

在 2022/08/22 15:25, Tejun Heo 写道:
> Hello,
> 
> On Mon, Aug 22, 2022 at 11:06:44AM +0800, Yu Kuai wrote:
>> While implementing the new method, I found that there seems to be a
>> misunderstanding here, the code seems to try to add flag to split bio
>> so that it won't be throttled again for bps limit, however:
>>
>> 1) for blk throttle, split bio is issued directly and will never be
>> throttled again, while orignal bio will go through throttle path again.
>> 2) if cloned bio is directed to a new disk, the flag is cleared anyway.
> 
> Doesn't that make the current code correct then? But you were seeing
> incorrect behaviors, right?

According to the commit message in commit 111be8839817 ("block-throttle:
avoid double charge"):

If the bio is cloned/split, we copy the flag to new bio too to avoid a
double charge.

Which make me think the split bio will be resubmitted, and after
implementing the new solution, I found that test result is not as
expected. After spending sometime figuring out what is wrong, I found
that split bio will be dispatched directly from caller, while orignal
bio will be resubmitted.

I guess commit 111be8839817 made a mistake, however, there should be
no problem because orignal bio is flagged already, and it's handled
correctly.

Anyway, I removed the code in __bio_clone() and check flag in
__bio_split_to_limits in my patch:
--- a/block/bio.c
+++ b/block/bio.c
@@ -760,8 +760,6 @@ EXPORT_SYMBOL(bio_put);
  static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp)
  {
         bio_set_flag(bio, BIO_CLONED);
-       if (bio_flagged(bio_src, BIO_THROTTLED))
-               bio_set_flag(bio, BIO_THROTTLED);
         bio->bi_ioprio = bio_src->bi_ioprio;
         bio->bi_iter = bio_src->bi_iter;

diff --git a/block/blk-merge.c b/block/blk-merge.c
index ff04e9290715..10330bb038ca 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -358,6 +358,13 @@ struct bio *__bio_split_to_limits(struct bio *bio, 
struct queue_limits *lim,
                 blkcg_bio_issue_init(split);
                 bio_chain(split, bio);
                 trace_block_split(split, bio->bi_iter.bi_sector);
+
+               /*
+                * original bio will be resubmited and throttled again, 
clear
+                * the iops flag so that it can be count again for iops 
limit.
+                */
+               if (bio_flagged(bio, BIO_IOPS_THROTTLED))
+                       bio_clear_flag(bio, BIO_IOPS_THROTTLED);
                 submit_bio_noacct(bio);
                 return split;
         }


> 
> Thanks.
> 

Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Yu Kuai]

Hi, Tejun

ÔÚ 2022/08/22 15:25, Tejun Heo дµÀ:
> Hello,
> 
> On Mon, Aug 22, 2022 at 11:06:44AM +0800, Yu Kuai wrote:
>> While implementing the new method, I found that there seems to be a
>> misunderstanding here, the code seems to try to add flag to split bio
>> so that it won't be throttled again for bps limit, however:
>>
>> 1) for blk throttle, split bio is issued directly and will never be
>> throttled again, while orignal bio will go through throttle path again.
>> 2) if cloned bio is directed to a new disk, the flag is cleared anyway.
> 
> Doesn't that make the current code correct then? But you were seeing
> incorrect behaviors, right?

According to the commit message in commit 111be8839817 ("block-throttle:
avoid double charge"):

If the bio is cloned/split, we copy the flag to new bio too to avoid a
double charge.

Which make me think the split bio will be resubmitted, and after
implementing the new solution, I found that test result is not as
expected. After spending sometime figuring out what is wrong, I found
that split bio will be dispatched directly from caller, while orignal
bio will be resubmitted.

I guess commit 111be8839817 made a mistake, however, there should be
no problem because orignal bio is flagged already, and it's handled
correctly.

Anyway, I removed the code in __bio_clone() and check flag in
__bio_split_to_limits in my patch:
--- a/block/bio.c
+++ b/block/bio.c
@@ -760,8 +760,6 @@ EXPORT_SYMBOL(bio_put);
  static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp)
  {
         bio_set_flag(bio, BIO_CLONED);
-       if (bio_flagged(bio_src, BIO_THROTTLED))
-               bio_set_flag(bio, BIO_THROTTLED);
         bio->bi_ioprio = bio_src->bi_ioprio;
         bio->bi_iter = bio_src->bi_iter;

diff --git a/block/blk-merge.c b/block/blk-merge.c
index ff04e9290715..10330bb038ca 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -358,6 +358,13 @@ struct bio *__bio_split_to_limits(struct bio *bio, 
struct queue_limits *lim,
                 blkcg_bio_issue_init(split);
                 bio_chain(split, bio);
                 trace_block_split(split, bio->bi_iter.bi_sector);
+
+               /*
+                * original bio will be resubmited and throttled again, 
clear
+                * the iops flag so that it can be count again for iops 
limit.
+                */
+               if (bio_flagged(bio, BIO_IOPS_THROTTLED))
+                       bio_clear_flag(bio, BIO_IOPS_THROTTLED);
                 submit_bio_noacct(bio);
                 return split;
         }


> 
> Thanks.
> 

[PATCH v7 2/9] blk-throttle: prevent overflow while calculating wait time

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

There is a problem found by code review in tg_with_in_bps_limit() that
'bps_limit * jiffy_elapsed_rnd' might overflow. Fix the problem by
calling mul_u64_u64_div_u64() instead.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 block/blk-throttle.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 2957e2c643f4..da1fae243321 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -806,7 +806,7 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
 				 u64 bps_limit, unsigned long *wait)
 {
 	bool rw = bio_data_dir(bio);
-	u64 bytes_allowed, extra_bytes, tmp;
+	u64 bytes_allowed, extra_bytes;
 	unsigned long jiffy_elapsed, jiffy_wait, jiffy_elapsed_rnd;
 	unsigned int bio_size = throtl_bio_data_size(bio);
 
@@ -824,10 +824,8 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
 		jiffy_elapsed_rnd = tg->td->throtl_slice;
 
 	jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, tg->td->throtl_slice);
-
-	tmp = bps_limit * jiffy_elapsed_rnd;
-	do_div(tmp, HZ);
-	bytes_allowed = tmp;
+	bytes_allowed = mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed_rnd,
+					    (u64)HZ);
 
 	if (tg->bytes_disp[rw] + bio_size <= bytes_allowed) {
 		if (wait)
-- 
2.31.1

[PATCH v7 3/9] blk-throttle: factor out code to calculate ios/bytes_allowed

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

No functional changes, new apis will be used in later patches to
calculate wait time for throttled bios when new configuration is
submitted.

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

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index da1fae243321..0d9719c41fe2 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -754,33 +754,20 @@ static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw)
 		   tg->slice_start[rw], tg->slice_end[rw], jiffies);
 }
 
-static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
-				  u32 iops_limit, unsigned long *wait)
+static unsigned int calculate_io_allowed(u32 iops_limit,
+					 unsigned long jiffy_elapsed)
 {
-	bool rw = bio_data_dir(bio);
 	unsigned int io_allowed;
-	unsigned long jiffy_elapsed, jiffy_wait, jiffy_elapsed_rnd;
 	u64 tmp;
 
-	if (iops_limit == UINT_MAX) {
-		if (wait)
-			*wait = 0;
-		return true;
-	}
-
-	jiffy_elapsed = jiffies - tg->slice_start[rw];
-
-	/* Round up to the next throttle slice, wait time must be nonzero */
-	jiffy_elapsed_rnd = roundup(jiffy_elapsed + 1, tg->td->throtl_slice);
-
 	/*
-	 * jiffy_elapsed_rnd should not be a big value as minimum iops can be
+	 * jiffy_elapsed should not be a big value as minimum iops can be
 	 * 1 then at max jiffy elapsed should be equivalent of 1 second as we
 	 * will allow dispatch after 1 second and after that slice should
 	 * have been trimmed.
 	 */
 
-	tmp = (u64)iops_limit * jiffy_elapsed_rnd;
+	tmp = (u64)iops_limit * jiffy_elapsed;
 	do_div(tmp, HZ);
 
 	if (tmp > UINT_MAX)
@@ -788,6 +775,32 @@ static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
 	else
 		io_allowed = tmp;
 
+	return io_allowed;
+}
+
+static u64 calculate_bytes_allowed(u64 bps_limit, unsigned long jiffy_elapsed)
+{
+	return mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed, (u64)HZ);
+}
+
+static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
+				  u32 iops_limit, unsigned long *wait)
+{
+	bool rw = bio_data_dir(bio);
+	unsigned int io_allowed;
+	unsigned long jiffy_elapsed, jiffy_wait, jiffy_elapsed_rnd;
+
+	if (iops_limit == UINT_MAX) {
+		if (wait)
+			*wait = 0;
+		return true;
+	}
+
+	jiffy_elapsed = jiffies - tg->slice_start[rw];
+
+	/* Round up to the next throttle slice, wait time must be nonzero */
+	jiffy_elapsed_rnd = roundup(jiffy_elapsed + 1, tg->td->throtl_slice);
+	io_allowed = calculate_io_allowed(iops_limit, jiffy_elapsed_rnd);
 	if (tg->io_disp[rw] + 1 <= io_allowed) {
 		if (wait)
 			*wait = 0;
@@ -824,9 +837,7 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
 		jiffy_elapsed_rnd = tg->td->throtl_slice;
 
 	jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, tg->td->throtl_slice);
-	bytes_allowed = mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed_rnd,
-					    (u64)HZ);
-
+	bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed_rnd);
 	if (tg->bytes_disp[rw] + bio_size <= bytes_allowed) {
 		if (wait)
 			*wait = 0;
-- 
2.31.1

[PATCH v7 3/9] blk-throttle: factor out code to calculate ios/bytes_allowed

[Yu Kuai]

From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

No functional changes, new apis will be used in later patches to
calculate wait time for throttled bios when new configuration is
submitted.

Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 block/blk-throttle.c | 51 +++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 20 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index da1fae243321..0d9719c41fe2 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -754,33 +754,20 @@ static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw)
 		   tg->slice_start[rw], tg->slice_end[rw], jiffies);
 }
 
-static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
-				  u32 iops_limit, unsigned long *wait)
+static unsigned int calculate_io_allowed(u32 iops_limit,
+					 unsigned long jiffy_elapsed)
 {
-	bool rw = bio_data_dir(bio);
 	unsigned int io_allowed;
-	unsigned long jiffy_elapsed, jiffy_wait, jiffy_elapsed_rnd;
 	u64 tmp;
 
-	if (iops_limit == UINT_MAX) {
-		if (wait)
-			*wait = 0;
-		return true;
-	}
-
-	jiffy_elapsed = jiffies - tg->slice_start[rw];
-
-	/* Round up to the next throttle slice, wait time must be nonzero */
-	jiffy_elapsed_rnd = roundup(jiffy_elapsed + 1, tg->td->throtl_slice);
-
 	/*
-	 * jiffy_elapsed_rnd should not be a big value as minimum iops can be
+	 * jiffy_elapsed should not be a big value as minimum iops can be
 	 * 1 then at max jiffy elapsed should be equivalent of 1 second as we
 	 * will allow dispatch after 1 second and after that slice should
 	 * have been trimmed.
 	 */
 
-	tmp = (u64)iops_limit * jiffy_elapsed_rnd;
+	tmp = (u64)iops_limit * jiffy_elapsed;
 	do_div(tmp, HZ);
 
 	if (tmp > UINT_MAX)
@@ -788,6 +775,32 @@ static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
 	else
 		io_allowed = tmp;
 
+	return io_allowed;
+}
+
+static u64 calculate_bytes_allowed(u64 bps_limit, unsigned long jiffy_elapsed)
+{
+	return mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed, (u64)HZ);
+}
+
+static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
+				  u32 iops_limit, unsigned long *wait)
+{
+	bool rw = bio_data_dir(bio);
+	unsigned int io_allowed;
+	unsigned long jiffy_elapsed, jiffy_wait, jiffy_elapsed_rnd;
+
+	if (iops_limit == UINT_MAX) {
+		if (wait)
+			*wait = 0;
+		return true;
+	}
+
+	jiffy_elapsed = jiffies - tg->slice_start[rw];
+
+	/* Round up to the next throttle slice, wait time must be nonzero */
+	jiffy_elapsed_rnd = roundup(jiffy_elapsed + 1, tg->td->throtl_slice);
+	io_allowed = calculate_io_allowed(iops_limit, jiffy_elapsed_rnd);
 	if (tg->io_disp[rw] + 1 <= io_allowed) {
 		if (wait)
 			*wait = 0;
@@ -824,9 +837,7 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
 		jiffy_elapsed_rnd = tg->td->throtl_slice;
 
 	jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, tg->td->throtl_slice);
-	bytes_allowed = mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed_rnd,
-					    (u64)HZ);
-
+	bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed_rnd);
 	if (tg->bytes_disp[rw] + bio_size <= bytes_allowed) {
 		if (wait)
 			*wait = 0;
-- 
2.31.1

Re: [PATCH v7 3/9] blk-throttle: factor out code to calculate ios/bytes_allowed

[Tejun Heo]

On Tue, Aug 02, 2022 at 10:04:09PM +0800, Yu Kuai wrote:
> +static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
> +				  u32 iops_limit, unsigned long *wait)

While at it, can you please rename these functions to tg_within_iops_limit?
"within" is a single word. Other than that,

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

Thanks.

-- 
tejun

Re: [PATCH v7 3/9] blk-throttle: factor out code to calculate ios/bytes_allowed

[Yu Kuai]

Hi, Tejun!

在 2022/08/17 3:47, Tejun Heo 写道:
> On Tue, Aug 02, 2022 at 10:04:09PM +0800, Yu Kuai wrote:
>> +static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
>> +				  u32 iops_limit, unsigned long *wait)
> 
> While at it, can you please rename these functions to tg_within_iops_limit?
> "within" is a single word. Other than that,

Of course, I'll do that in next version.
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> Thanks.
> 

Re: [PATCH v7 3/9] blk-throttle: factor out code to calculate ios/bytes_allowed

[Yu Kuai]

Hi, Tejun!

ÔÚ 2022/08/17 3:47, Tejun Heo Ğ´µÀ:
> On Tue, Aug 02, 2022 at 10:04:09PM +0800, Yu Kuai wrote:
>> +static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
>> +				  u32 iops_limit, unsigned long *wait)
> 
> While at it, can you please rename these functions to tg_within_iops_limit?
> "within" is a single word. Other than that,

Of course, I'll do that in next version.
> 
> Acked-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 
> Thanks.
> 

[PATCH v7 4/9] blk-throttle: fix io hung due to configuration updates

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

If new configuration is submitted while a bio is throttled, then new
waiting time is recalculated regardless that the bio might already wait
for some time:

tg_conf_updated
 throtl_start_new_slice
  tg_update_disptime
  throtl_schedule_next_dispatch

Then io hung can be triggered by always submmiting new configuration
before the throttled bio is dispatched.

Fix the problem by respecting the time that throttled bio already waited.
In order to do that, add new fields to record how many bytes/io are
waited, and use it to calculate wait time for throttled bio under new
configuration.

Some simple test:
1)
cd /sys/fs/cgroup/blkio/
echo $$ > cgroup.procs
echo "8:0 2048" > blkio.throttle.write_bps_device
{
        sleep 2
        echo "8:0 1024" > blkio.throttle.write_bps_device
} &
dd if=/dev/zero of=/dev/sda bs=8k count=1 oflag=direct

2)
cd /sys/fs/cgroup/blkio/
echo $$ > cgroup.procs
echo "8:0 1024" > blkio.throttle.write_bps_device
{
        sleep 4
        echo "8:0 2048" > blkio.throttle.write_bps_device
} &
dd if=/dev/zero of=/dev/sda bs=8k count=1 oflag=direct

test results: io finish time
	before this patch	with this patch
1)	10s			6s
2)	8s			6s

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Reviewed-by: Michal Koutný <mkoutny@suse.com>
---
 block/blk-throttle.c | 63 +++++++++++++++++++++++++++++++++++++++-----
 block/blk-throttle.h | 11 ++++++++
 2 files changed, 68 insertions(+), 6 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 0d9719c41fe2..621402cf2576 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -639,6 +639,8 @@ static inline void throtl_start_new_slice_with_credit(struct throtl_grp *tg,
 {
 	tg->bytes_disp[rw] = 0;
 	tg->io_disp[rw] = 0;
+	tg->bytes_skipped[rw] = 0;
+	tg->io_skipped[rw] = 0;
 
 	/*
 	 * Previous slice has expired. We must have trimmed it after last
@@ -656,12 +658,17 @@ static inline void throtl_start_new_slice_with_credit(struct throtl_grp *tg,
 		   tg->slice_end[rw], jiffies);
 }
 
-static inline void throtl_start_new_slice(struct throtl_grp *tg, bool rw)
+static inline void throtl_start_new_slice(struct throtl_grp *tg, bool rw,
+					  bool clear_skipped)
 {
 	tg->bytes_disp[rw] = 0;
 	tg->io_disp[rw] = 0;
 	tg->slice_start[rw] = jiffies;
 	tg->slice_end[rw] = jiffies + tg->td->throtl_slice;
+	if (clear_skipped) {
+		tg->bytes_skipped[rw] = 0;
+		tg->io_skipped[rw] = 0;
+	}
 
 	throtl_log(&tg->service_queue,
 		   "[%c] new slice start=%lu end=%lu jiffies=%lu",
@@ -783,6 +790,46 @@ static u64 calculate_bytes_allowed(u64 bps_limit, unsigned long jiffy_elapsed)
 	return mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed, (u64)HZ);
 }
 
+static void __tg_update_skipped(struct throtl_grp *tg, bool rw)
+{
+	unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw];
+	u64 bps_limit = tg_bps_limit(tg, rw);
+	u32 iops_limit = tg_iops_limit(tg, rw);
+
+	/*
+	 * If config is updated while bios are still throttled, calculate and
+	 * accumulate how many bytes/io are waited across changes. And
+	 * bytes/io_skipped will be used to calculate new wait time under new
+	 * configuration.
+	 *
+	 * Following calculation won't overflow as long as bios that are
+	 * dispatched later won't preempt already throttled bios. Even if such
+	 * overflow do happen, there should be no problem because unsigned is
+	 * used here, and bytes_skipped/io_skipped will be updated correctly.
+	 */
+	if (bps_limit != U64_MAX)
+		tg->bytes_skipped[rw] +=
+			calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
+			tg->bytes_disp[rw];
+	if (iops_limit != UINT_MAX)
+		tg->io_skipped[rw] +=
+			calculate_io_allowed(iops_limit, jiffy_elapsed) -
+			tg->io_disp[rw];
+}
+
+static void tg_update_skipped(struct throtl_grp *tg)
+{
+	if (tg->service_queue.nr_queued[READ])
+		__tg_update_skipped(tg, READ);
+	if (tg->service_queue.nr_queued[WRITE])
+		__tg_update_skipped(tg, WRITE);
+
+	/* see comments in struct throtl_grp for meaning of these fields. */
+	throtl_log(&tg->service_queue, "%s: %llu %llu %u %u\n", __func__,
+		   tg->bytes_skipped[READ], tg->bytes_skipped[WRITE],
+		   tg->io_skipped[READ], tg->io_skipped[WRITE]);
+}
+
 static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
 				  u32 iops_limit, unsigned long *wait)
 {
@@ -800,7 +847,8 @@ static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
 
 	/* Round up to the next throttle slice, wait time must be nonzero */
 	jiffy_elapsed_rnd = roundup(jiffy_elapsed + 1, tg->td->throtl_slice);
-	io_allowed = calculate_io_allowed(iops_limit, jiffy_elapsed_rnd);
+	io_allowed = calculate_io_allowed(iops_limit, jiffy_elapsed_rnd) +
+		     tg->io_skipped[rw];
 	if (tg->io_disp[rw] + 1 <= io_allowed) {
 		if (wait)
 			*wait = 0;
@@ -837,7 +885,8 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
 		jiffy_elapsed_rnd = tg->td->throtl_slice;
 
 	jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, tg->td->throtl_slice);
-	bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed_rnd);
+	bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed_rnd) +
+			tg->bytes_skipped[rw];
 	if (tg->bytes_disp[rw] + bio_size <= bytes_allowed) {
 		if (wait)
 			*wait = 0;
@@ -898,7 +947,7 @@ static bool tg_may_dispatch(struct throtl_grp *tg, struct bio *bio,
 	 * slice and it should be extended instead.
 	 */
 	if (throtl_slice_used(tg, rw) && !(tg->service_queue.nr_queued[rw]))
-		throtl_start_new_slice(tg, rw);
+		throtl_start_new_slice(tg, rw, true);
 	else {
 		if (time_before(tg->slice_end[rw],
 		    jiffies + tg->td->throtl_slice))
@@ -1327,8 +1376,8 @@ static void tg_conf_updated(struct throtl_grp *tg, bool global)
 	 * that a group's limit are dropped suddenly and we don't want to
 	 * account recently dispatched IO with new low rate.
 	 */
-	throtl_start_new_slice(tg, READ);
-	throtl_start_new_slice(tg, WRITE);
+	throtl_start_new_slice(tg, READ, false);
+	throtl_start_new_slice(tg, WRITE, false);
 
 	if (tg->flags & THROTL_TG_PENDING) {
 		tg_update_disptime(tg);
@@ -1356,6 +1405,7 @@ static ssize_t tg_set_conf(struct kernfs_open_file *of,
 		v = U64_MAX;
 
 	tg = blkg_to_tg(ctx.blkg);
+	tg_update_skipped(tg);
 
 	if (is_u64)
 		*(u64 *)((void *)tg + of_cft(of)->private) = v;
@@ -1542,6 +1592,7 @@ static ssize_t tg_set_limit(struct kernfs_open_file *of,
 		return ret;
 
 	tg = blkg_to_tg(ctx.blkg);
+	tg_update_skipped(tg);
 
 	v[0] = tg->bps_conf[READ][index];
 	v[1] = tg->bps_conf[WRITE][index];
diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index c1b602996127..0163aa9104c3 100644
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -115,6 +115,17 @@ struct throtl_grp {
 	uint64_t bytes_disp[2];
 	/* Number of bio's dispatched in current slice */
 	unsigned int io_disp[2];
+	/*
+	 * The following two fields are updated when new configuration is
+	 * submitted while some bios are still throttled, they record how many
+	 * bytes/io are waited already in previous configuration, and they will
+	 * be used to calculate wait time under new configuration.
+	 *
+	 * Number of bytes will be skipped in current slice
+	 */
+	uint64_t bytes_skipped[2];
+	/* Number of bio will be skipped in current slice */
+	unsigned int io_skipped[2];
 
 	unsigned long last_low_overflow_time[2];
 
-- 
2.31.1

[PATCH v7 4/9] blk-throttle: fix io hung due to configuration updates

[Yu Kuai]

From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

If new configuration is submitted while a bio is throttled, then new
waiting time is recalculated regardless that the bio might already wait
for some time:

tg_conf_updated
 throtl_start_new_slice
  tg_update_disptime
  throtl_schedule_next_dispatch

Then io hung can be triggered by always submmiting new configuration
before the throttled bio is dispatched.

Fix the problem by respecting the time that throttled bio already waited.
In order to do that, add new fields to record how many bytes/io are
waited, and use it to calculate wait time for throttled bio under new
configuration.

Some simple test:
1)
cd /sys/fs/cgroup/blkio/
echo $$ > cgroup.procs
echo "8:0 2048" > blkio.throttle.write_bps_device
{
        sleep 2
        echo "8:0 1024" > blkio.throttle.write_bps_device
} &
dd if=/dev/zero of=/dev/sda bs=8k count=1 oflag=direct

2)
cd /sys/fs/cgroup/blkio/
echo $$ > cgroup.procs
echo "8:0 1024" > blkio.throttle.write_bps_device
{
        sleep 4
        echo "8:0 2048" > blkio.throttle.write_bps_device
} &
dd if=/dev/zero of=/dev/sda bs=8k count=1 oflag=direct

test results: io finish time
	before this patch	with this patch
1)	10s			6s
2)	8s			6s

Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Michal Koutn√Ω <mkoutny-IBi9RG/b67k@public.gmane.org>
---
 block/blk-throttle.c | 63 +++++++++++++++++++++++++++++++++++++++-----
 block/blk-throttle.h | 11 ++++++++
 2 files changed, 68 insertions(+), 6 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 0d9719c41fe2..621402cf2576 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -639,6 +639,8 @@ static inline void throtl_start_new_slice_with_credit(struct throtl_grp *tg,
 {
 	tg->bytes_disp[rw] = 0;
 	tg->io_disp[rw] = 0;
+	tg->bytes_skipped[rw] = 0;
+	tg->io_skipped[rw] = 0;
 
 	/*
 	 * Previous slice has expired. We must have trimmed it after last
@@ -656,12 +658,17 @@ static inline void throtl_start_new_slice_with_credit(struct throtl_grp *tg,
 		   tg->slice_end[rw], jiffies);
 }
 
-static inline void throtl_start_new_slice(struct throtl_grp *tg, bool rw)
+static inline void throtl_start_new_slice(struct throtl_grp *tg, bool rw,
+					  bool clear_skipped)
 {
 	tg->bytes_disp[rw] = 0;
 	tg->io_disp[rw] = 0;
 	tg->slice_start[rw] = jiffies;
 	tg->slice_end[rw] = jiffies + tg->td->throtl_slice;
+	if (clear_skipped) {
+		tg->bytes_skipped[rw] = 0;
+		tg->io_skipped[rw] = 0;
+	}
 
 	throtl_log(&tg->service_queue,
 		   "[%c] new slice start=%lu end=%lu jiffies=%lu",
@@ -783,6 +790,46 @@ static u64 calculate_bytes_allowed(u64 bps_limit, unsigned long jiffy_elapsed)
 	return mul_u64_u64_div_u64(bps_limit, (u64)jiffy_elapsed, (u64)HZ);
 }
 
+static void __tg_update_skipped(struct throtl_grp *tg, bool rw)
+{
+	unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw];
+	u64 bps_limit = tg_bps_limit(tg, rw);
+	u32 iops_limit = tg_iops_limit(tg, rw);
+
+	/*
+	 * If config is updated while bios are still throttled, calculate and
+	 * accumulate how many bytes/io are waited across changes. And
+	 * bytes/io_skipped will be used to calculate new wait time under new
+	 * configuration.
+	 *
+	 * Following calculation won't overflow as long as bios that are
+	 * dispatched later won't preempt already throttled bios. Even if such
+	 * overflow do happen, there should be no problem because unsigned is
+	 * used here, and bytes_skipped/io_skipped will be updated correctly.
+	 */
+	if (bps_limit != U64_MAX)
+		tg->bytes_skipped[rw] +=
+			calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
+			tg->bytes_disp[rw];
+	if (iops_limit != UINT_MAX)
+		tg->io_skipped[rw] +=
+			calculate_io_allowed(iops_limit, jiffy_elapsed) -
+			tg->io_disp[rw];
+}
+
+static void tg_update_skipped(struct throtl_grp *tg)
+{
+	if (tg->service_queue.nr_queued[READ])
+		__tg_update_skipped(tg, READ);
+	if (tg->service_queue.nr_queued[WRITE])
+		__tg_update_skipped(tg, WRITE);
+
+	/* see comments in struct throtl_grp for meaning of these fields. */
+	throtl_log(&tg->service_queue, "%s: %llu %llu %u %u\n", __func__,
+		   tg->bytes_skipped[READ], tg->bytes_skipped[WRITE],
+		   tg->io_skipped[READ], tg->io_skipped[WRITE]);
+}
+
 static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
 				  u32 iops_limit, unsigned long *wait)
 {
@@ -800,7 +847,8 @@ static bool tg_with_in_iops_limit(struct throtl_grp *tg, struct bio *bio,
 
 	/* Round up to the next throttle slice, wait time must be nonzero */
 	jiffy_elapsed_rnd = roundup(jiffy_elapsed + 1, tg->td->throtl_slice);
-	io_allowed = calculate_io_allowed(iops_limit, jiffy_elapsed_rnd);
+	io_allowed = calculate_io_allowed(iops_limit, jiffy_elapsed_rnd) +
+		     tg->io_skipped[rw];
 	if (tg->io_disp[rw] + 1 <= io_allowed) {
 		if (wait)
 			*wait = 0;
@@ -837,7 +885,8 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
 		jiffy_elapsed_rnd = tg->td->throtl_slice;
 
 	jiffy_elapsed_rnd = roundup(jiffy_elapsed_rnd, tg->td->throtl_slice);
-	bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed_rnd);
+	bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed_rnd) +
+			tg->bytes_skipped[rw];
 	if (tg->bytes_disp[rw] + bio_size <= bytes_allowed) {
 		if (wait)
 			*wait = 0;
@@ -898,7 +947,7 @@ static bool tg_may_dispatch(struct throtl_grp *tg, struct bio *bio,
 	 * slice and it should be extended instead.
 	 */
 	if (throtl_slice_used(tg, rw) && !(tg->service_queue.nr_queued[rw]))
-		throtl_start_new_slice(tg, rw);
+		throtl_start_new_slice(tg, rw, true);
 	else {
 		if (time_before(tg->slice_end[rw],
 		    jiffies + tg->td->throtl_slice))
@@ -1327,8 +1376,8 @@ static void tg_conf_updated(struct throtl_grp *tg, bool global)
 	 * that a group's limit are dropped suddenly and we don't want to
 	 * account recently dispatched IO with new low rate.
 	 */
-	throtl_start_new_slice(tg, READ);
-	throtl_start_new_slice(tg, WRITE);
+	throtl_start_new_slice(tg, READ, false);
+	throtl_start_new_slice(tg, WRITE, false);
 
 	if (tg->flags & THROTL_TG_PENDING) {
 		tg_update_disptime(tg);
@@ -1356,6 +1405,7 @@ static ssize_t tg_set_conf(struct kernfs_open_file *of,
 		v = U64_MAX;
 
 	tg = blkg_to_tg(ctx.blkg);
+	tg_update_skipped(tg);
 
 	if (is_u64)
 		*(u64 *)((void *)tg + of_cft(of)->private) = v;
@@ -1542,6 +1592,7 @@ static ssize_t tg_set_limit(struct kernfs_open_file *of,
 		return ret;
 
 	tg = blkg_to_tg(ctx.blkg);
+	tg_update_skipped(tg);
 
 	v[0] = tg->bps_conf[READ][index];
 	v[1] = tg->bps_conf[WRITE][index];
diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index c1b602996127..0163aa9104c3 100644
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -115,6 +115,17 @@ struct throtl_grp {
 	uint64_t bytes_disp[2];
 	/* Number of bio's dispatched in current slice */
 	unsigned int io_disp[2];
+	/*
+	 * The following two fields are updated when new configuration is
+	 * submitted while some bios are still throttled, they record how many
+	 * bytes/io are waited already in previous configuration, and they will
+	 * be used to calculate wait time under new configuration.
+	 *
+	 * Number of bytes will be skipped in current slice
+	 */
+	uint64_t bytes_skipped[2];
+	/* Number of bio will be skipped in current slice */
+	unsigned int io_skipped[2];
 
 	unsigned long last_low_overflow_time[2];
 
-- 
2.31.1

Re: [PATCH v7 4/9] blk-throttle: fix io hung due to configuration updates

[Tejun Heo]

On Tue, Aug 02, 2022 at 10:04:10PM +0800, Yu Kuai wrote:
...
> +static void __tg_update_skipped(struct throtl_grp *tg, bool rw)
> +{
> +	unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw];
> +	u64 bps_limit = tg_bps_limit(tg, rw);
> +	u32 iops_limit = tg_iops_limit(tg, rw);
> +
> +	/*
> +	 * If config is updated while bios are still throttled, calculate and
> +	 * accumulate how many bytes/io are waited across changes. And
> +	 * bytes/io_skipped will be used to calculate new wait time under new
> +	 * configuration.
> +	 *
> +	 * Following calculation won't overflow as long as bios that are
> +	 * dispatched later won't preempt already throttled bios. Even if such
> +	 * overflow do happen, there should be no problem because unsigned is
> +	 * used here, and bytes_skipped/io_skipped will be updated correctly.
> +	 */

Would it be easier if the fields were signed? It's fragile and odd to
explain "these are unsigned but if they underflow they behave just like
signed when added" when they can just be signed. Also, I have a hard time
understand what "preempt" means above.

> +	if (bps_limit != U64_MAX)
> +		tg->bytes_skipped[rw] +=
> +			calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
> +			tg->bytes_disp[rw];
> +	if (iops_limit != UINT_MAX)
> +		tg->io_skipped[rw] +=
> +			calculate_io_allowed(iops_limit, jiffy_elapsed) -
> +			tg->io_disp[rw];

So, this is calculating the budgets to carry over. Can we name them
accordingly? I don't know what "skipped" means.

> @@ -115,6 +115,17 @@ struct throtl_grp {
>  	uint64_t bytes_disp[2];
>  	/* Number of bio's dispatched in current slice */
>  	unsigned int io_disp[2];
> +	/*
> +	 * The following two fields are updated when new configuration is
> +	 * submitted while some bios are still throttled, they record how many
> +	 * bytes/io are waited already in previous configuration, and they will
> +	 * be used to calculate wait time under new configuration.
> +	 *
> +	 * Number of bytes will be skipped in current slice
> +	 */
> +	uint64_t bytes_skipped[2];
> +	/* Number of bio will be skipped in current slice */
> +	unsigned int io_skipped[2];

So, the code seems to make sense but the field names and comments don't
really, at least to me. I can't find an intuitive understanding of what's
being skipped. Can you please take another stab at making this more
understandable?

Thanks.

-- 
tejun

Re: [PATCH v7 4/9] blk-throttle: fix io hung due to configuration updates

[Tejun Heo]

On Tue, Aug 02, 2022 at 10:04:10PM +0800, Yu Kuai wrote:
...
> +static void __tg_update_skipped(struct throtl_grp *tg, bool rw)
> +{
> +	unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw];
> +	u64 bps_limit = tg_bps_limit(tg, rw);
> +	u32 iops_limit = tg_iops_limit(tg, rw);
> +
> +	/*
> +	 * If config is updated while bios are still throttled, calculate and
> +	 * accumulate how many bytes/io are waited across changes. And
> +	 * bytes/io_skipped will be used to calculate new wait time under new
> +	 * configuration.
> +	 *
> +	 * Following calculation won't overflow as long as bios that are
> +	 * dispatched later won't preempt already throttled bios. Even if such
> +	 * overflow do happen, there should be no problem because unsigned is
> +	 * used here, and bytes_skipped/io_skipped will be updated correctly.
> +	 */

Would it be easier if the fields were signed? It's fragile and odd to
explain "these are unsigned but if they underflow they behave just like
signed when added" when they can just be signed. Also, I have a hard time
understand what "preempt" means above.

> +	if (bps_limit != U64_MAX)
> +		tg->bytes_skipped[rw] +=
> +			calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
> +			tg->bytes_disp[rw];
> +	if (iops_limit != UINT_MAX)
> +		tg->io_skipped[rw] +=
> +			calculate_io_allowed(iops_limit, jiffy_elapsed) -
> +			tg->io_disp[rw];

So, this is calculating the budgets to carry over. Can we name them
accordingly? I don't know what "skipped" means.

> @@ -115,6 +115,17 @@ struct throtl_grp {
>  	uint64_t bytes_disp[2];
>  	/* Number of bio's dispatched in current slice */
>  	unsigned int io_disp[2];
> +	/*
> +	 * The following two fields are updated when new configuration is
> +	 * submitted while some bios are still throttled, they record how many
> +	 * bytes/io are waited already in previous configuration, and they will
> +	 * be used to calculate wait time under new configuration.
> +	 *
> +	 * Number of bytes will be skipped in current slice
> +	 */
> +	uint64_t bytes_skipped[2];
> +	/* Number of bio will be skipped in current slice */
> +	unsigned int io_skipped[2];

So, the code seems to make sense but the field names and comments don't
really, at least to me. I can't find an intuitive understanding of what's
being skipped. Can you please take another stab at making this more
understandable?

Thanks.

-- 
tejun

Re: [PATCH v7 4/9] blk-throttle: fix io hung due to configuration updates

[Yu Kuai]

Hi, Tejun!

在 2022/08/17 4:01, Tejun Heo 写道:
> On Tue, Aug 02, 2022 at 10:04:10PM +0800, Yu Kuai wrote:
> ...
>> +static void __tg_update_skipped(struct throtl_grp *tg, bool rw)
>> +{
>> +	unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw];
>> +	u64 bps_limit = tg_bps_limit(tg, rw);
>> +	u32 iops_limit = tg_iops_limit(tg, rw);
>> +
>> +	/*
>> +	 * If config is updated while bios are still throttled, calculate and
>> +	 * accumulate how many bytes/io are waited across changes. And
>> +	 * bytes/io_skipped will be used to calculate new wait time under new
>> +	 * configuration.
>> +	 *
>> +	 * Following calculation won't overflow as long as bios that are
>> +	 * dispatched later won't preempt already throttled bios. Even if such
>> +	 * overflow do happen, there should be no problem because unsigned is
>> +	 * used here, and bytes_skipped/io_skipped will be updated correctly.
>> +	 */
> 
> Would it be easier if the fields were signed? It's fragile and odd to
> explain "these are unsigned but if they underflow they behave just like
> signed when added" when they can just be signed. Also, I have a hard time
> understand what "preempt" means above.

I think preempt shound never happen based on current FIFO
implementation, perhaps
> 
>> +	if (bps_limit != U64_MAX)
>> +		tg->bytes_skipped[rw] +=
>> +			calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
>> +			tg->bytes_disp[rw];
>> +	if (iops_limit != UINT_MAX)
>> +		tg->io_skipped[rw] +=
>> +			calculate_io_allowed(iops_limit, jiffy_elapsed) -
>> +			tg->io_disp[rw];
> 
> So, this is calculating the budgets to carry over. Can we name them
> accordingly? I don't know what "skipped" means.

Yeah, thanks for you advice, art of naming is a little hard for me...
How do you think about these name: extended_bytes/io_budget?
> 
>> @@ -115,6 +115,17 @@ struct throtl_grp {
>>   	uint64_t bytes_disp[2];
>>   	/* Number of bio's dispatched in current slice */
>>   	unsigned int io_disp[2];
>> +	/*
>> +	 * The following two fields are updated when new configuration is
>> +	 * submitted while some bios are still throttled, they record how many
>> +	 * bytes/io are waited already in previous configuration, and they will
>> +	 * be used to calculate wait time under new configuration.
>> +	 *
>> +	 * Number of bytes will be skipped in current slice
>> +	 */
>> +	uint64_t bytes_skipped[2];
>> +	/* Number of bio will be skipped in current slice */
>> +	unsigned int io_skipped[2];
> 
> So, the code seems to make sense but the field names and comments don't
> really, at least to me. I can't find an intuitive understanding of what's
> being skipped. Can you please take another stab at making this more
> understandable?
> 
> Thanks.
> 

Re: [PATCH v7 4/9] blk-throttle: fix io hung due to configuration updates

[Yu Kuai]

Hi, Tejun!

ÔÚ 2022/08/17 4:01, Tejun Heo дµÀ:
> On Tue, Aug 02, 2022 at 10:04:10PM +0800, Yu Kuai wrote:
> ...
>> +static void __tg_update_skipped(struct throtl_grp *tg, bool rw)
>> +{
>> +	unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw];
>> +	u64 bps_limit = tg_bps_limit(tg, rw);
>> +	u32 iops_limit = tg_iops_limit(tg, rw);
>> +
>> +	/*
>> +	 * If config is updated while bios are still throttled, calculate and
>> +	 * accumulate how many bytes/io are waited across changes. And
>> +	 * bytes/io_skipped will be used to calculate new wait time under new
>> +	 * configuration.
>> +	 *
>> +	 * Following calculation won't overflow as long as bios that are
>> +	 * dispatched later won't preempt already throttled bios. Even if such
>> +	 * overflow do happen, there should be no problem because unsigned is
>> +	 * used here, and bytes_skipped/io_skipped will be updated correctly.
>> +	 */
> 
> Would it be easier if the fields were signed? It's fragile and odd to
> explain "these are unsigned but if they underflow they behave just like
> signed when added" when they can just be signed. Also, I have a hard time
> understand what "preempt" means above.

I think preempt shound never happen based on current FIFO
implementation, perhaps
> 
>> +	if (bps_limit != U64_MAX)
>> +		tg->bytes_skipped[rw] +=
>> +			calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
>> +			tg->bytes_disp[rw];
>> +	if (iops_limit != UINT_MAX)
>> +		tg->io_skipped[rw] +=
>> +			calculate_io_allowed(iops_limit, jiffy_elapsed) -
>> +			tg->io_disp[rw];
> 
> So, this is calculating the budgets to carry over. Can we name them
> accordingly? I don't know what "skipped" means.

Yeah, thanks for you advice, art of naming is a little hard for me...
How do you think about these name: extended_bytes/io_budget?
> 
>> @@ -115,6 +115,17 @@ struct throtl_grp {
>>   	uint64_t bytes_disp[2];
>>   	/* Number of bio's dispatched in current slice */
>>   	unsigned int io_disp[2];
>> +	/*
>> +	 * The following two fields are updated when new configuration is
>> +	 * submitted while some bios are still throttled, they record how many
>> +	 * bytes/io are waited already in previous configuration, and they will
>> +	 * be used to calculate wait time under new configuration.
>> +	 *
>> +	 * Number of bytes will be skipped in current slice
>> +	 */
>> +	uint64_t bytes_skipped[2];
>> +	/* Number of bio will be skipped in current slice */
>> +	unsigned int io_skipped[2];
> 
> So, the code seems to make sense but the field names and comments don't
> really, at least to me. I can't find an intuitive understanding of what's
> being skipped. Can you please take another stab at making this more
> understandable?
> 
> Thanks.
> 

Re: [PATCH v7 4/9] blk-throttle: fix io hung due to configuration updates

[Tejun Heo]

On Wed, Aug 17, 2022 at 09:30:30AM +0800, Yu Kuai wrote:
> > Would it be easier if the fields were signed? It's fragile and odd to
> > explain "these are unsigned but if they underflow they behave just like
> > signed when added" when they can just be signed. Also, I have a hard time
> > understand what "preempt" means above.
> 
> I think preempt shound never happen based on current FIFO
> implementation, perhaps

Can you elaborate what "preempt" is?

> > > +	if (bps_limit != U64_MAX)
> > > +		tg->bytes_skipped[rw] +=
> > > +			calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
> > > +			tg->bytes_disp[rw];
> > > +	if (iops_limit != UINT_MAX)
> > > +		tg->io_skipped[rw] +=
> > > +			calculate_io_allowed(iops_limit, jiffy_elapsed) -
> > > +			tg->io_disp[rw];
> > 
> > So, this is calculating the budgets to carry over. Can we name them
> > accordingly? I don't know what "skipped" means.
> 
> Yeah, thanks for you advice, art of naming is a little hard for me...
> How do you think about these name: extended_bytes/io_budget?

How about carryover_{ios|bytes}?

Thanks.

-- 
tejun

Re: [PATCH v7 4/9] blk-throttle: fix io hung due to configuration updates

[Tejun Heo]

On Wed, Aug 17, 2022 at 09:30:30AM +0800, Yu Kuai wrote:
> > Would it be easier if the fields were signed? It's fragile and odd to
> > explain "these are unsigned but if they underflow they behave just like
> > signed when added" when they can just be signed. Also, I have a hard time
> > understand what "preempt" means above.
> 
> I think preempt shound never happen based on current FIFO
> implementation, perhaps

Can you elaborate what "preempt" is?

> > > +	if (bps_limit != U64_MAX)
> > > +		tg->bytes_skipped[rw] +=
> > > +			calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
> > > +			tg->bytes_disp[rw];
> > > +	if (iops_limit != UINT_MAX)
> > > +		tg->io_skipped[rw] +=
> > > +			calculate_io_allowed(iops_limit, jiffy_elapsed) -
> > > +			tg->io_disp[rw];
> > 
> > So, this is calculating the budgets to carry over. Can we name them
> > accordingly? I don't know what "skipped" means.
> 
> Yeah, thanks for you advice, art of naming is a little hard for me...
> How do you think about these name: extended_bytes/io_budget?

How about carryover_{ios|bytes}?

Thanks.

-- 
tejun

Re: [PATCH v7 4/9] blk-throttle: fix io hung due to configuration updates

[Yu Kuai]

Hi, Tejun!

在 2022/08/18 1:52, Tejun Heo 写道:
> On Wed, Aug 17, 2022 at 09:30:30AM +0800, Yu Kuai wrote:
>>> Would it be easier if the fields were signed? It's fragile and odd to
>>> explain "these are unsigned but if they underflow they behave just like
>>> signed when added" when they can just be signed. Also, I have a hard time
>>> understand what "preempt" means above.
>>
>> I think preempt shound never happen based on current FIFO
>> implementation, perhaps
> 
> Can you elaborate what "preempt" is?

Here preempt means that the bio that is throttled later somehow get
dispatched earlier, Michal thinks it's better to comment that the code
still works fine in this particular scenario.

> 
>>>> +	if (bps_limit != U64_MAX)
>>>> +		tg->bytes_skipped[rw] +=
>>>> +			calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
>>>> +			tg->bytes_disp[rw];
>>>> +	if (iops_limit != UINT_MAX)
>>>> +		tg->io_skipped[rw] +=
>>>> +			calculate_io_allowed(iops_limit, jiffy_elapsed) -
>>>> +			tg->io_disp[rw];
>>>
>>> So, this is calculating the budgets to carry over. Can we name them
>>> accordingly? I don't know what "skipped" means.
>>
>> Yeah, thanks for you advice, art of naming is a little hard for me...
>> How do you think about these name: extended_bytes/io_budget?
> 
> How about carryover_{ios|bytes}?

Yes, that sounds good.

By the way, should I use 'ios' here instead of 'io'? I was confused
because there are many places that is using 'io' currently.

Thanks,
Kuai
> 
> Thanks.
> 

Re: [PATCH v7 4/9] blk-throttle: fix io hung due to configuration updates

[Yu Kuai]

Hi, Tejun!

ÔÚ 2022/08/18 1:52, Tejun Heo дµÀ:
> On Wed, Aug 17, 2022 at 09:30:30AM +0800, Yu Kuai wrote:
>>> Would it be easier if the fields were signed? It's fragile and odd to
>>> explain "these are unsigned but if they underflow they behave just like
>>> signed when added" when they can just be signed. Also, I have a hard time
>>> understand what "preempt" means above.
>>
>> I think preempt shound never happen based on current FIFO
>> implementation, perhaps
> 
> Can you elaborate what "preempt" is?

Here preempt means that the bio that is throttled later somehow get
dispatched earlier, Michal thinks it's better to comment that the code
still works fine in this particular scenario.

> 
>>>> +	if (bps_limit != U64_MAX)
>>>> +		tg->bytes_skipped[rw] +=
>>>> +			calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
>>>> +			tg->bytes_disp[rw];
>>>> +	if (iops_limit != UINT_MAX)
>>>> +		tg->io_skipped[rw] +=
>>>> +			calculate_io_allowed(iops_limit, jiffy_elapsed) -
>>>> +			tg->io_disp[rw];
>>>
>>> So, this is calculating the budgets to carry over. Can we name them
>>> accordingly? I don't know what "skipped" means.
>>
>> Yeah, thanks for you advice, art of naming is a little hard for me...
>> How do you think about these name: extended_bytes/io_budget?
> 
> How about carryover_{ios|bytes}?

Yes, that sounds good.

By the way, should I use 'ios' here instead of 'io'? I was confused
because there are many places that is using 'io' currently.

Thanks,
Kuai
> 
> Thanks.
> 

Re: [PATCH v7 4/9] blk-throttle: fix io hung due to configuration updates

[Tejun Heo]

Hello,

On Thu, Aug 18, 2022 at 09:16:28AM +0800, Yu Kuai wrote:
> 在 2022/08/18 1:52, Tejun Heo 写道:
> > On Wed, Aug 17, 2022 at 09:30:30AM +0800, Yu Kuai wrote:
> > > > Would it be easier if the fields were signed? It's fragile and odd to
> > > > explain "these are unsigned but if they underflow they behave just like
> > > > signed when added" when they can just be signed. Also, I have a hard time
> > > > understand what "preempt" means above.
> > > 
> > > I think preempt shound never happen based on current FIFO
> > > implementation, perhaps
> > 
> > Can you elaborate what "preempt" is?
> 
> Here preempt means that the bio that is throttled later somehow get
> dispatched earlier, Michal thinks it's better to comment that the code
> still works fine in this particular scenario.

You'd have to spell it out. It's not clear "preempt" means the above.

> > How about carryover_{ios|bytes}?
> 
> Yes, that sounds good.
> 
> By the way, should I use 'ios' here instead of 'io'? I was confused
> because there are many places that is using 'io' currently.

Yeah, blk-throttle.c is kinda inconsistent about that. It uses bytes/ios in
some places and bytes/io in others. I'd prefer ios here.

Thanks.

-- 
tejun

Re: [PATCH v7 4/9] blk-throttle: fix io hung due to configuration updates

[Tejun Heo]

Hello,

On Thu, Aug 18, 2022 at 09:16:28AM +0800, Yu Kuai wrote:
> 在 2022/08/18 1:52, Tejun Heo 写道:
> > On Wed, Aug 17, 2022 at 09:30:30AM +0800, Yu Kuai wrote:
> > > > Would it be easier if the fields were signed? It's fragile and odd to
> > > > explain "these are unsigned but if they underflow they behave just like
> > > > signed when added" when they can just be signed. Also, I have a hard time
> > > > understand what "preempt" means above.
> > > 
> > > I think preempt shound never happen based on current FIFO
> > > implementation, perhaps
> > 
> > Can you elaborate what "preempt" is?
> 
> Here preempt means that the bio that is throttled later somehow get
> dispatched earlier, Michal thinks it's better to comment that the code
> still works fine in this particular scenario.

You'd have to spell it out. It's not clear "preempt" means the above.

> > How about carryover_{ios|bytes}?
> 
> Yes, that sounds good.
> 
> By the way, should I use 'ios' here instead of 'io'? I was confused
> because there are many places that is using 'io' currently.

Yeah, blk-throttle.c is kinda inconsistent about that. It uses bytes/ios in
some places and bytes/io in others. I'd prefer ios here.

Thanks.

-- 
tejun

[PATCH v7 5/9] blk-throttle: improve handling of re-entered bio for bps limit

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Currently, if new slice is started while bio is re-entered, such bio
will count multiple times for bps limit. Since 'bytes_skipped'
represents how many bytes will be skipped while dispatching bios, which
can handle that case, increasing it instead of decreasing 'bytes_disp'.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-throttle.c | 8 ++++----
 block/blk-throttle.h | 4 +++-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 621402cf2576..237668328e98 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -2183,14 +2183,14 @@ bool __blk_throtl_bio(struct bio *bio)
 		 * Splited bios can be re-entered because iops limit should be
 		 * counted again, however, bps limit should not. Since bps limit
 		 * will be counted again while dispatching it, compensate the
-		 * over-accounting here. Noted that compensation can fail if
-		 * new slice is started.
+		 * over-accounting here. Since decrement of 'bytes_disp' can't
+		 * handle the case that new slice is started, increase
+		 * 'bytes_skipped' instead.
 		 */
 		if (bio_flagged(bio, BIO_THROTTLED)) {
 			unsigned int bio_size = throtl_bio_data_size(bio);
 
-			if (tg->bytes_disp[rw] >= bio_size)
-				tg->bytes_disp[rw] -= bio_size;
+			tg->bytes_skipped[rw] += bio_size;
 			if (tg->last_bytes_disp[rw] >= bio_size)
 				tg->last_bytes_disp[rw] -= bio_size;
 		}
diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index 0163aa9104c3..c9545616ba12 100644
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -121,7 +121,9 @@ struct throtl_grp {
 	 * bytes/io are waited already in previous configuration, and they will
 	 * be used to calculate wait time under new configuration.
 	 *
-	 * Number of bytes will be skipped in current slice
+	 * Number of bytes will be skipped in current slice. In addition, this
+	 * field will help to handle re-entered bio for bps limit, see details
+	 * in __blk_throtl_bio().
 	 */
 	uint64_t bytes_skipped[2];
 	/* Number of bio will be skipped in current slice */
-- 
2.31.1

[PATCH v7 5/9] blk-throttle: improve handling of re-entered bio for bps limit

[Yu Kuai]

From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Currently, if new slice is started while bio is re-entered, such bio
will count multiple times for bps limit. Since 'bytes_skipped'
represents how many bytes will be skipped while dispatching bios, which
can handle that case, increasing it instead of decreasing 'bytes_disp'.

Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 block/blk-throttle.c | 8 ++++----
 block/blk-throttle.h | 4 +++-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 621402cf2576..237668328e98 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -2183,14 +2183,14 @@ bool __blk_throtl_bio(struct bio *bio)
 		 * Splited bios can be re-entered because iops limit should be
 		 * counted again, however, bps limit should not. Since bps limit
 		 * will be counted again while dispatching it, compensate the
-		 * over-accounting here. Noted that compensation can fail if
-		 * new slice is started.
+		 * over-accounting here. Since decrement of 'bytes_disp' can't
+		 * handle the case that new slice is started, increase
+		 * 'bytes_skipped' instead.
 		 */
 		if (bio_flagged(bio, BIO_THROTTLED)) {
 			unsigned int bio_size = throtl_bio_data_size(bio);
 
-			if (tg->bytes_disp[rw] >= bio_size)
-				tg->bytes_disp[rw] -= bio_size;
+			tg->bytes_skipped[rw] += bio_size;
 			if (tg->last_bytes_disp[rw] >= bio_size)
 				tg->last_bytes_disp[rw] -= bio_size;
 		}
diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index 0163aa9104c3..c9545616ba12 100644
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -121,7 +121,9 @@ struct throtl_grp {
 	 * bytes/io are waited already in previous configuration, and they will
 	 * be used to calculate wait time under new configuration.
 	 *
-	 * Number of bytes will be skipped in current slice
+	 * Number of bytes will be skipped in current slice. In addition, this
+	 * field will help to handle re-entered bio for bps limit, see details
+	 * in __blk_throtl_bio().
 	 */
 	uint64_t bytes_skipped[2];
 	/* Number of bio will be skipped in current slice */
-- 
2.31.1

Re: [PATCH v7 5/9] blk-throttle: improve handling of re-entered bio for bps limit

[Tejun Heo]

On Tue, Aug 02, 2022 at 10:04:11PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Currently, if new slice is started while bio is re-entered, such bio
> will count multiple times for bps limit. Since 'bytes_skipped'
> represents how many bytes will be skipped while dispatching bios, which
> can handle that case, increasing it instead of decreasing 'bytes_disp'.

The same thing. I think we're working around the problem in the wrong place.

Thanks.

-- 
tejun

Re: [PATCH v7 5/9] blk-throttle: improve handling of re-entered bio for bps limit

[Tejun Heo]

On Tue, Aug 02, 2022 at 10:04:11PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> 
> Currently, if new slice is started while bio is re-entered, such bio
> will count multiple times for bps limit. Since 'bytes_skipped'
> represents how many bytes will be skipped while dispatching bios, which
> can handle that case, increasing it instead of decreasing 'bytes_disp'.

The same thing. I think we're working around the problem in the wrong place.

Thanks.

-- 
tejun

[PATCH v7 6/9] blk-throttle: use 'READ/WRITE' instead of '0/1'

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Make the code easier to read, like everywhere else.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 block/blk-throttle.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 237668328e98..907001eede85 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -329,8 +329,8 @@ static struct bio *throtl_pop_queued(struct list_head *queued,
 /* init a service_queue, assumes the caller zeroed it */
 static void throtl_service_queue_init(struct throtl_service_queue *sq)
 {
-	INIT_LIST_HEAD(&sq->queued[0]);
-	INIT_LIST_HEAD(&sq->queued[1]);
+	INIT_LIST_HEAD(&sq->queued[READ]);
+	INIT_LIST_HEAD(&sq->queued[WRITE]);
 	sq->pending_tree = RB_ROOT_CACHED;
 	timer_setup(&sq->pending_timer, throtl_pending_timer_fn, 0);
 }
@@ -1161,7 +1161,7 @@ static int throtl_select_dispatch(struct throtl_service_queue *parent_sq)
 		nr_disp += throtl_dispatch_tg(tg);
 
 		sq = &tg->service_queue;
-		if (sq->nr_queued[0] || sq->nr_queued[1])
+		if (sq->nr_queued[READ] || sq->nr_queued[WRITE])
 			tg_update_disptime(tg);
 
 		if (nr_disp >= THROTL_QUANTUM)
-- 
2.31.1

[PATCH v7 6/9] blk-throttle: use 'READ/WRITE' instead of '0/1'

[Yu Kuai]

From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Make the code easier to read, like everywhere else.

Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Acked-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 block/blk-throttle.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 237668328e98..907001eede85 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -329,8 +329,8 @@ static struct bio *throtl_pop_queued(struct list_head *queued,
 /* init a service_queue, assumes the caller zeroed it */
 static void throtl_service_queue_init(struct throtl_service_queue *sq)
 {
-	INIT_LIST_HEAD(&sq->queued[0]);
-	INIT_LIST_HEAD(&sq->queued[1]);
+	INIT_LIST_HEAD(&sq->queued[READ]);
+	INIT_LIST_HEAD(&sq->queued[WRITE]);
 	sq->pending_tree = RB_ROOT_CACHED;
 	timer_setup(&sq->pending_timer, throtl_pending_timer_fn, 0);
 }
@@ -1161,7 +1161,7 @@ static int throtl_select_dispatch(struct throtl_service_queue *parent_sq)
 		nr_disp += throtl_dispatch_tg(tg);
 
 		sq = &tg->service_queue;
-		if (sq->nr_queued[0] || sq->nr_queued[1])
+		if (sq->nr_queued[READ] || sq->nr_queued[WRITE])
 			tg_update_disptime(tg);
 
 		if (nr_disp >= THROTL_QUANTUM)
-- 
2.31.1

Re: [PATCH v7 6/9] blk-throttle: use 'READ/WRITE' instead of '0/1'

[Tejun Heo]

On Tue, Aug 02, 2022 at 10:04:12PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Make the code easier to read, like everywhere else.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> Acked-by: Tejun Heo <tj@kernel.org>

Let's float trivial acked patches to the front so that they can applied
without waiting for the rest of the series.

Thanks.

-- 
tejun

Re: [PATCH v7 6/9] blk-throttle: use 'READ/WRITE' instead of '0/1'

[Yu Kuai]

Hi, Tejun

在 2022/08/17 4:03, Tejun Heo 写道:
> On Tue, Aug 02, 2022 at 10:04:12PM +0800, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Make the code easier to read, like everywhere else.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> Acked-by: Tejun Heo <tj@kernel.org>
> 
> Let's float trivial acked patches to the front so that they can applied
> without waiting for the rest of the series.
> 
Good point! I'll move last 4 patches into a new patchset.

Thanks,
Kuai
> Thanks.
> 

Re: [PATCH v7 6/9] blk-throttle: use 'READ/WRITE' instead of '0/1'

[Yu Kuai]

Hi, Tejun

ÔÚ 2022/08/17 4:03, Tejun Heo дµÀ:
> On Tue, Aug 02, 2022 at 10:04:12PM +0800, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Make the code easier to read, like everywhere else.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> Acked-by: Tejun Heo <tj@kernel.org>
> 
> Let's float trivial acked patches to the front so that they can applied
> without waiting for the rest of the series.
> 
Good point! I'll move last 4 patches into a new patchset.

Thanks,
Kuai
> Thanks.
> 

[PATCH v7 7/9] blk-throttle: calling throtl_dequeue/enqueue_tg in pairs

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

It's a little weird to call throtl_dequeue_tg() unconditionally in
throtl_select_dispatch(), since it will be called in tg_update_disptime()
again if some bio is still throttled. Thus call it later if there are no
throttled bio. There are no functional changes.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 block/blk-throttle.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 907001eede85..f7048f87b19f 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1156,13 +1156,13 @@ static int throtl_select_dispatch(struct throtl_service_queue *parent_sq)
 		if (time_before(jiffies, tg->disptime))
 			break;
 
-		throtl_dequeue_tg(tg);
-
 		nr_disp += throtl_dispatch_tg(tg);
 
 		sq = &tg->service_queue;
 		if (sq->nr_queued[READ] || sq->nr_queued[WRITE])
 			tg_update_disptime(tg);
+		else
+			throtl_dequeue_tg(tg);
 
 		if (nr_disp >= THROTL_QUANTUM)
 			break;
-- 
2.31.1

[PATCH v7 7/9] blk-throttle: calling throtl_dequeue/enqueue_tg in pairs

[Yu Kuai]

From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

It's a little weird to call throtl_dequeue_tg() unconditionally in
throtl_select_dispatch(), since it will be called in tg_update_disptime()
again if some bio is still throttled. Thus call it later if there are no
throttled bio. There are no functional changes.

Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Acked-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 block/blk-throttle.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 907001eede85..f7048f87b19f 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1156,13 +1156,13 @@ static int throtl_select_dispatch(struct throtl_service_queue *parent_sq)
 		if (time_before(jiffies, tg->disptime))
 			break;
 
-		throtl_dequeue_tg(tg);
-
 		nr_disp += throtl_dispatch_tg(tg);
 
 		sq = &tg->service_queue;
 		if (sq->nr_queued[READ] || sq->nr_queued[WRITE])
 			tg_update_disptime(tg);
+		else
+			throtl_dequeue_tg(tg);
 
 		if (nr_disp >= THROTL_QUANTUM)
 			break;
-- 
2.31.1

[PATCH v7 8/9] blk-throttle: cleanup tg_update_disptime()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

tg_update_disptime() only need to adjust postion for 'tg' in
'parent_sq', there is no need to call throtl_enqueue/dequeue_tg().

Save a little overhead in tg_update_disptime() and prepare to cleanup
flag 'THROTL_TG_PENDING', there are no functional changes.

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

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index f7048f87b19f..6b2096e95221 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -520,7 +520,6 @@ static void throtl_rb_erase(struct rb_node *n,
 {
 	rb_erase_cached(n, &parent_sq->pending_tree);
 	RB_CLEAR_NODE(n);
-	--parent_sq->nr_pending;
 }
 
 static void update_min_dispatch_time(struct throtl_service_queue *parent_sq)
@@ -572,7 +571,11 @@ static void throtl_enqueue_tg(struct throtl_grp *tg)
 static void throtl_dequeue_tg(struct throtl_grp *tg)
 {
 	if (tg->flags & THROTL_TG_PENDING) {
-		throtl_rb_erase(&tg->rb_node, tg->service_queue.parent_sq);
+		struct throtl_service_queue *parent_sq =
+			tg->service_queue.parent_sq;
+
+		throtl_rb_erase(&tg->rb_node, parent_sq);
+		--parent_sq->nr_pending;
 		tg->flags &= ~THROTL_TG_PENDING;
 	}
 }
@@ -1045,9 +1048,9 @@ static void tg_update_disptime(struct throtl_grp *tg)
 	disptime = jiffies + min_wait;
 
 	/* Update dispatch time */
-	throtl_dequeue_tg(tg);
+	throtl_rb_erase(&tg->rb_node, tg->service_queue.parent_sq);
 	tg->disptime = disptime;
-	throtl_enqueue_tg(tg);
+	tg_service_queue_add(tg);
 
 	/* see throtl_add_bio_tg() */
 	tg->flags &= ~THROTL_TG_WAS_EMPTY;
-- 
2.31.1

[PATCH v7 8/9] blk-throttle: cleanup tg_update_disptime()

[Yu Kuai]

From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

tg_update_disptime() only need to adjust postion for 'tg' in
'parent_sq', there is no need to call throtl_enqueue/dequeue_tg().

Save a little overhead in tg_update_disptime() and prepare to cleanup
flag 'THROTL_TG_PENDING', there are no functional changes.

Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 block/blk-throttle.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index f7048f87b19f..6b2096e95221 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -520,7 +520,6 @@ static void throtl_rb_erase(struct rb_node *n,
 {
 	rb_erase_cached(n, &parent_sq->pending_tree);
 	RB_CLEAR_NODE(n);
-	--parent_sq->nr_pending;
 }
 
 static void update_min_dispatch_time(struct throtl_service_queue *parent_sq)
@@ -572,7 +571,11 @@ static void throtl_enqueue_tg(struct throtl_grp *tg)
 static void throtl_dequeue_tg(struct throtl_grp *tg)
 {
 	if (tg->flags & THROTL_TG_PENDING) {
-		throtl_rb_erase(&tg->rb_node, tg->service_queue.parent_sq);
+		struct throtl_service_queue *parent_sq =
+			tg->service_queue.parent_sq;
+
+		throtl_rb_erase(&tg->rb_node, parent_sq);
+		--parent_sq->nr_pending;
 		tg->flags &= ~THROTL_TG_PENDING;
 	}
 }
@@ -1045,9 +1048,9 @@ static void tg_update_disptime(struct throtl_grp *tg)
 	disptime = jiffies + min_wait;
 
 	/* Update dispatch time */
-	throtl_dequeue_tg(tg);
+	throtl_rb_erase(&tg->rb_node, tg->service_queue.parent_sq);
 	tg->disptime = disptime;
-	throtl_enqueue_tg(tg);
+	tg_service_queue_add(tg);
 
 	/* see throtl_add_bio_tg() */
 	tg->flags &= ~THROTL_TG_WAS_EMPTY;
-- 
2.31.1

Re: [PATCH v7 8/9] blk-throttle: cleanup tg_update_disptime()

[Tejun Heo]

On Tue, Aug 02, 2022 at 10:04:14PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> tg_update_disptime() only need to adjust postion for 'tg' in
> 'parent_sq', there is no need to call throtl_enqueue/dequeue_tg().
> 
> Save a little overhead in tg_update_disptime() and prepare to cleanup
> flag 'THROTL_TG_PENDING', there are no functional changes.

Does this actually help anything? Given that the heavy part of the operation
remains the same, this might not be much of an optimization. Is there even a
microbench that can show the difference?

Thanks.

-- 
tejun

Re: [PATCH v7 8/9] blk-throttle: cleanup tg_update_disptime()

[Yu Kuai]

Hi, Tejun！

在 2022/08/17 4:09, Tejun Heo 写道:
> On Tue, Aug 02, 2022 at 10:04:14PM +0800, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> tg_update_disptime() only need to adjust postion for 'tg' in
>> 'parent_sq', there is no need to call throtl_enqueue/dequeue_tg().
>>
>> Save a little overhead in tg_update_disptime() and prepare to cleanup
>> flag 'THROTL_TG_PENDING', there are no functional changes.
> 
> Does this actually help anything? Given that the heavy part of the operation
> remains the same, this might not be much of an optimization. Is there even a
> microbench that can show the difference?

It's right heavy part remains the same, the patch just remove some
unnecessary operations. And I didn't run benchmark to test that yet.

Thanks,
Kuai
> 
> Thanks.
> 

Re: [PATCH v7 8/9] blk-throttle: cleanup tg_update_disptime()

[Yu Kuai]

Hi, Tejun£¡

ÔÚ 2022/08/17 4:09, Tejun Heo дµÀ:
> On Tue, Aug 02, 2022 at 10:04:14PM +0800, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> tg_update_disptime() only need to adjust postion for 'tg' in
>> 'parent_sq', there is no need to call throtl_enqueue/dequeue_tg().
>>
>> Save a little overhead in tg_update_disptime() and prepare to cleanup
>> flag 'THROTL_TG_PENDING', there are no functional changes.
> 
> Does this actually help anything? Given that the heavy part of the operation
> remains the same, this might not be much of an optimization. Is there even a
> microbench that can show the difference?

It's right heavy part remains the same, the patch just remove some
unnecessary operations. And I didn't run benchmark to test that yet.

Thanks,
Kuai
> 
> Thanks.
> 

[PATCH v7 9/9] blk-throttle: clean up flag 'THROTL_TG_PENDING'

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

All related operations are inside 'queue_lock', there is no need to use
the flag, we only need to make sure throtl_enqueue_tg() is called when
the first bio is throttled, and throtl_dequeue_tg() is called when the
last throttled bio is dispatched. There are no functional changes in
this patch.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-throttle.c | 22 ++++++++--------------
 block/blk-throttle.h |  7 +++----
 2 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 6b2096e95221..778c0131adb1 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -561,23 +561,16 @@ static void tg_service_queue_add(struct throtl_grp *tg)
 
 static void throtl_enqueue_tg(struct throtl_grp *tg)
 {
-	if (!(tg->flags & THROTL_TG_PENDING)) {
-		tg_service_queue_add(tg);
-		tg->flags |= THROTL_TG_PENDING;
-		tg->service_queue.parent_sq->nr_pending++;
-	}
+	tg_service_queue_add(tg);
+	tg->service_queue.parent_sq->nr_pending++;
 }
 
 static void throtl_dequeue_tg(struct throtl_grp *tg)
 {
-	if (tg->flags & THROTL_TG_PENDING) {
-		struct throtl_service_queue *parent_sq =
-			tg->service_queue.parent_sq;
+	struct throtl_service_queue *parent_sq = tg->service_queue.parent_sq;
 
-		throtl_rb_erase(&tg->rb_node, parent_sq);
-		--parent_sq->nr_pending;
-		tg->flags &= ~THROTL_TG_PENDING;
-	}
+	throtl_rb_erase(&tg->rb_node, parent_sq);
+	--parent_sq->nr_pending;
 }
 
 /* Call with queue lock held */
@@ -1026,8 +1019,9 @@ static void throtl_add_bio_tg(struct bio *bio, struct throtl_qnode *qn,
 
 	throtl_qnode_add_bio(bio, qn, &sq->queued[rw]);
 
+	if (!sq->nr_queued[READ] && !sq->nr_queued[WRITE])
+		throtl_enqueue_tg(tg);
 	sq->nr_queued[rw]++;
-	throtl_enqueue_tg(tg);
 }
 
 static void tg_update_disptime(struct throtl_grp *tg)
@@ -1382,7 +1376,7 @@ static void tg_conf_updated(struct throtl_grp *tg, bool global)
 	throtl_start_new_slice(tg, READ, false);
 	throtl_start_new_slice(tg, WRITE, false);
 
-	if (tg->flags & THROTL_TG_PENDING) {
+	if (sq->nr_queued[READ] || sq->nr_queued[WRITE]) {
 		tg_update_disptime(tg);
 		throtl_schedule_next_dispatch(sq->parent_sq, true);
 	}
diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index c9545616ba12..2ae5ac8fe76e 100644
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -53,10 +53,9 @@ struct throtl_service_queue {
 };
 
 enum tg_state_flags {
-	THROTL_TG_PENDING	= 1 << 0,	/* on parent's pending tree */
-	THROTL_TG_WAS_EMPTY	= 1 << 1,	/* bio_lists[] became non-empty */
-	THROTL_TG_HAS_IOPS_LIMIT = 1 << 2,	/* tg has iops limit */
-	THROTL_TG_CANCELING	= 1 << 3,	/* starts to cancel bio */
+	THROTL_TG_WAS_EMPTY	= 1 << 0,	/* bio_lists[] became non-empty */
+	THROTL_TG_HAS_IOPS_LIMIT = 1 << 1,	/* tg has iops limit */
+	THROTL_TG_CANCELING	= 1 << 2,	/* starts to cancel bio */
 };
 
 enum {
-- 
2.31.1

[PATCH v7 9/9] blk-throttle: clean up flag 'THROTL_TG_PENDING'

[Yu Kuai]

From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

All related operations are inside 'queue_lock', there is no need to use
the flag, we only need to make sure throtl_enqueue_tg() is called when
the first bio is throttled, and throtl_dequeue_tg() is called when the
last throttled bio is dispatched. There are no functional changes in
this patch.

Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 block/blk-throttle.c | 22 ++++++++--------------
 block/blk-throttle.h |  7 +++----
 2 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 6b2096e95221..778c0131adb1 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -561,23 +561,16 @@ static void tg_service_queue_add(struct throtl_grp *tg)
 
 static void throtl_enqueue_tg(struct throtl_grp *tg)
 {
-	if (!(tg->flags & THROTL_TG_PENDING)) {
-		tg_service_queue_add(tg);
-		tg->flags |= THROTL_TG_PENDING;
-		tg->service_queue.parent_sq->nr_pending++;
-	}
+	tg_service_queue_add(tg);
+	tg->service_queue.parent_sq->nr_pending++;
 }
 
 static void throtl_dequeue_tg(struct throtl_grp *tg)
 {
-	if (tg->flags & THROTL_TG_PENDING) {
-		struct throtl_service_queue *parent_sq =
-			tg->service_queue.parent_sq;
+	struct throtl_service_queue *parent_sq = tg->service_queue.parent_sq;
 
-		throtl_rb_erase(&tg->rb_node, parent_sq);
-		--parent_sq->nr_pending;
-		tg->flags &= ~THROTL_TG_PENDING;
-	}
+	throtl_rb_erase(&tg->rb_node, parent_sq);
+	--parent_sq->nr_pending;
 }
 
 /* Call with queue lock held */
@@ -1026,8 +1019,9 @@ static void throtl_add_bio_tg(struct bio *bio, struct throtl_qnode *qn,
 
 	throtl_qnode_add_bio(bio, qn, &sq->queued[rw]);
 
+	if (!sq->nr_queued[READ] && !sq->nr_queued[WRITE])
+		throtl_enqueue_tg(tg);
 	sq->nr_queued[rw]++;
-	throtl_enqueue_tg(tg);
 }
 
 static void tg_update_disptime(struct throtl_grp *tg)
@@ -1382,7 +1376,7 @@ static void tg_conf_updated(struct throtl_grp *tg, bool global)
 	throtl_start_new_slice(tg, READ, false);
 	throtl_start_new_slice(tg, WRITE, false);
 
-	if (tg->flags & THROTL_TG_PENDING) {
+	if (sq->nr_queued[READ] || sq->nr_queued[WRITE]) {
 		tg_update_disptime(tg);
 		throtl_schedule_next_dispatch(sq->parent_sq, true);
 	}
diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index c9545616ba12..2ae5ac8fe76e 100644
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -53,10 +53,9 @@ struct throtl_service_queue {
 };
 
 enum tg_state_flags {
-	THROTL_TG_PENDING	= 1 << 0,	/* on parent's pending tree */
-	THROTL_TG_WAS_EMPTY	= 1 << 1,	/* bio_lists[] became non-empty */
-	THROTL_TG_HAS_IOPS_LIMIT = 1 << 2,	/* tg has iops limit */
-	THROTL_TG_CANCELING	= 1 << 3,	/* starts to cancel bio */
+	THROTL_TG_WAS_EMPTY	= 1 << 0,	/* bio_lists[] became non-empty */
+	THROTL_TG_HAS_IOPS_LIMIT = 1 << 1,	/* tg has iops limit */
+	THROTL_TG_CANCELING	= 1 << 2,	/* starts to cancel bio */
 };
 
 enum {
-- 
2.31.1

Re: [PATCH v7 9/9] blk-throttle: clean up flag 'THROTL_TG_PENDING'

[Tejun Heo]

On Tue, Aug 02, 2022 at 10:04:15PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> All related operations are inside 'queue_lock', there is no need to use
> the flag, we only need to make sure throtl_enqueue_tg() is called when
> the first bio is throttled, and throtl_dequeue_tg() is called when the
> last throttled bio is dispatched. There are no functional changes in
> this patch.

I don't know whether this is better or not. It's minutely less lines of code
but also makes the code a bit more fragile. I'm ambivalent. At any rate,
please move these trivial patches to the head of the series or post them
separately.

Thanks.

-- 
tejun

Re: [PATCH v7 9/9] blk-throttle: clean up flag 'THROTL_TG_PENDING'

[Yu Kuai]

Hi, Tejun!

在 2022/08/17 4:14, Tejun Heo 写道:
> On Tue, Aug 02, 2022 at 10:04:15PM +0800, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> All related operations are inside 'queue_lock', there is no need to use
>> the flag, we only need to make sure throtl_enqueue_tg() is called when
>> the first bio is throttled, and throtl_dequeue_tg() is called when the
>> last throttled bio is dispatched. There are no functional changes in
>> this patch.
> 
> I don't know whether this is better or not. It's minutely less lines of code
> but also makes the code a bit more fragile. I'm ambivalent. At any rate,
> please move these trivial patches to the head of the series or post them
> separately.

Can I ask why do you think this patch makes the code a bit more fragile?

By the way, I'll post these trivial patches separately.

Thanks,
Kuai
> 
> Thanks.
> 

Re: [PATCH v7 9/9] blk-throttle: clean up flag 'THROTL_TG_PENDING'

[Yu Kuai]

Hi, Tejun!

ÔÚ 2022/08/17 4:14, Tejun Heo дµÀ:
> On Tue, Aug 02, 2022 at 10:04:15PM +0800, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>
>> All related operations are inside 'queue_lock', there is no need to use
>> the flag, we only need to make sure throtl_enqueue_tg() is called when
>> the first bio is throttled, and throtl_dequeue_tg() is called when the
>> last throttled bio is dispatched. There are no functional changes in
>> this patch.
> 
> I don't know whether this is better or not. It's minutely less lines of code
> but also makes the code a bit more fragile. I'm ambivalent. At any rate,
> please move these trivial patches to the head of the series or post them
> separately.

Can I ask why do you think this patch makes the code a bit more fragile?

By the way, I'll post these trivial patches separately.

Thanks,
Kuai
> 
> Thanks.
> 

Re: [PATCH v7 9/9] blk-throttle: clean up flag 'THROTL_TG_PENDING'

[Tejun Heo]

Hello,

On Wed, Aug 17, 2022 at 09:45:13AM +0800, Yu Kuai wrote:
> > I don't know whether this is better or not. It's minutely less lines of code
> > but also makes the code a bit more fragile. I'm ambivalent. At any rate,
> > please move these trivial patches to the head of the series or post them
> > separately.
> 
> Can I ask why do you think this patch makes the code a bit more fragile?

It's just one step further removed. Before, the flag was trivially in sync
with the on queue status. After, the relationship is more indirect and
easier to break accidentally. Not that it's a major problem. Just not sure
what the benefit of the change is.

> By the way, I'll post these trivial patches separately.

Sounds great.

Thanks.

-- 
tejun

Re: [PATCH v7 9/9] blk-throttle: clean up flag 'THROTL_TG_PENDING'

[Tejun Heo]

Hello,

On Wed, Aug 17, 2022 at 09:45:13AM +0800, Yu Kuai wrote:
> > I don't know whether this is better or not. It's minutely less lines of code
> > but also makes the code a bit more fragile. I'm ambivalent. At any rate,
> > please move these trivial patches to the head of the series or post them
> > separately.
> 
> Can I ask why do you think this patch makes the code a bit more fragile?

It's just one step further removed. Before, the flag was trivially in sync
with the on queue status. After, the relationship is more indirect and
easier to break accidentally. Not that it's a major problem. Just not sure
what the benefit of the change is.

> By the way, I'll post these trivial patches separately.

Sounds great.

Thanks.

-- 
tejun

Re: [PATCH v7 9/9] blk-throttle: clean up flag 'THROTL_TG_PENDING'

[Yu Kuai]

Hi, Tejun!

在 2022/08/18 1:54, Tejun Heo 写道:
> Hello,
> 
> On Wed, Aug 17, 2022 at 09:45:13AM +0800, Yu Kuai wrote:
>>> I don't know whether this is better or not. It's minutely less lines of code
>>> but also makes the code a bit more fragile. I'm ambivalent. At any rate,
>>> please move these trivial patches to the head of the series or post them
>>> separately.
>>
>> Can I ask why do you think this patch makes the code a bit more fragile?
> 
> It's just one step further removed. Before, the flag was trivially in sync
> with the on queue status. After, the relationship is more indirect and
> easier to break accidentally. Not that it's a major problem. Just not sure
> what the benefit of the change is.

If you are worried about that, I can keep the flag, then the last two
patches will cleanup:

Before, the flag will be set and cleared frequently when each each bio
is handled.

After, the flag will only set then the first bio is throttled, and
it's cleared when last bio is dispatched.

Of course, if you think this cleanup is not necessary, I'll drop the
last two patches.

Thanks,
Kuai
> 
>> By the way, I'll post these trivial patches separately.
> 
> Sounds great.
> 
> Thanks.
> 

Re: [PATCH v7 9/9] blk-throttle: clean up flag 'THROTL_TG_PENDING'

[Yu Kuai]

Hi, Tejun!

ÔÚ 2022/08/18 1:54, Tejun Heo дµÀ:
> Hello,
> 
> On Wed, Aug 17, 2022 at 09:45:13AM +0800, Yu Kuai wrote:
>>> I don't know whether this is better or not. It's minutely less lines of code
>>> but also makes the code a bit more fragile. I'm ambivalent. At any rate,
>>> please move these trivial patches to the head of the series or post them
>>> separately.
>>
>> Can I ask why do you think this patch makes the code a bit more fragile?
> 
> It's just one step further removed. Before, the flag was trivially in sync
> with the on queue status. After, the relationship is more indirect and
> easier to break accidentally. Not that it's a major problem. Just not sure
> what the benefit of the change is.

If you are worried about that, I can keep the flag, then the last two
patches will cleanup:

Before, the flag will be set and cleared frequently when each each bio
is handled.

After, the flag will only set then the first bio is throttled, and
it's cleared when last bio is dispatched.

Of course, if you think this cleanup is not necessary, I'll drop the
last two patches.

Thanks,
Kuai
> 
>> By the way, I'll post these trivial patches separately.
> 
> Sounds great.
> 
> Thanks.
> 

Re: [PATCH v7 9/9] blk-throttle: clean up flag 'THROTL_TG_PENDING'

[Tejun Heo]

On Thu, Aug 18, 2022 at 05:29:39PM +0800, Yu Kuai wrote:
> Hi, Tejun!
> 
> 在 2022/08/18 1:54, Tejun Heo 写道:
> > Hello,
> > 
> > On Wed, Aug 17, 2022 at 09:45:13AM +0800, Yu Kuai wrote:
> > > > I don't know whether this is better or not. It's minutely less lines of code
> > > > but also makes the code a bit more fragile. I'm ambivalent. At any rate,
> > > > please move these trivial patches to the head of the series or post them
> > > > separately.
> > > 
> > > Can I ask why do you think this patch makes the code a bit more fragile?
> > 
> > It's just one step further removed. Before, the flag was trivially in sync
> > with the on queue status. After, the relationship is more indirect and
> > easier to break accidentally. Not that it's a major problem. Just not sure
> > what the benefit of the change is.
> 
> If you are worried about that, I can keep the flag, then the last two
> patches will cleanup:

I wasn't necessarily worried. It's more that I couldn't tell why the code is
better afterwards. Maybe update the commit message to explain why the new
code is better?

Thanks.

-- 
tejun

Re: [PATCH v7 9/9] blk-throttle: clean up flag 'THROTL_TG_PENDING'

[Tejun Heo]

On Thu, Aug 18, 2022 at 05:29:39PM +0800, Yu Kuai wrote:
> Hi, Tejun!
> 
> 在 2022/08/18 1:54, Tejun Heo 写道:
> > Hello,
> > 
> > On Wed, Aug 17, 2022 at 09:45:13AM +0800, Yu Kuai wrote:
> > > > I don't know whether this is better or not. It's minutely less lines of code
> > > > but also makes the code a bit more fragile. I'm ambivalent. At any rate,
> > > > please move these trivial patches to the head of the series or post them
> > > > separately.
> > > 
> > > Can I ask why do you think this patch makes the code a bit more fragile?
> > 
> > It's just one step further removed. Before, the flag was trivially in sync
> > with the on queue status. After, the relationship is more indirect and
> > easier to break accidentally. Not that it's a major problem. Just not sure
> > what the benefit of the change is.
> 
> If you are worried about that, I can keep the flag, then the last two
> patches will cleanup:

I wasn't necessarily worried. It's more that I couldn't tell why the code is
better afterwards. Maybe update the commit message to explain why the new
code is better?

Thanks.

-- 
tejun

Re: [PATCH v7 0/9] bugfix and cleanup for blk-throttle

[Yu Kuai]

在 2022/08/02 22:04, Yu Kuai 写道:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Changes in v7:
>   - add patch 5 to improve handling of re-entered bio for bps limit
>   - as suggested by Tejun, add some comments
>   - sdd some Acked tag by Tejun
> Changes in v6:
>   - rename parameter in patch 3
>   - add comments and reviewed tag for patch 4
> Changes in v5:
>   - add comments in patch 4
>   - clear bytes/io_skipped in throtl_start_new_slice_with_credit() in
>   patch 4
>   - and cleanup patches 5-8
> Changes in v4:
>   - add reviewed-by tag for patch 1
>   - add patch 2,3
>   - use a different way to fix io hung in patch 4
> Changes in v3:
>   - fix a check in patch 1
>   - fix link err in patch 2 on 32-bit platform
>   - handle overflow in patch 2
> Changes in v2:
>   - use a new solution suggested by Ming
>   - change the title of patch 1
>   - add patch 2
> 
> Patch 1 fix that blk-throttle can't work if multiple bios are throttle.
> Patch 2 fix overflow while calculating wait time.
> Patch 3,4 fix io hung due to configuration updates.
> Patch 5 improve handling of re-entered bio for bps limit.
> Patch 6-9 are cleanup patches, there are no functional changes, just
> some places that I think can be optimized during code review.
> 
friendly ping ...
> Previous version:
> v1: https://lore.kernel.org/all/20220517134909.2910251-1-yukuai3@huawei.com/
> v2: https://lore.kernel.org/all/20220518072751.1188163-1-yukuai3@huawei.com/
> v3: https://lore.kernel.org/all/20220519085811.879097-1-yukuai3@huawei.com/
> v4: https://lore.kernel.org/all/20220523082633.2324980-1-yukuai3@huawei.com/
> v5: https://lore.kernel.org/all/20220528064330.3471000-1-yukuai3@huawei.com/
> v6: https://lore.kernel.org/all/20220701093441.885741-1-yukuai1@huaweicloud.com/
> 
> Yu Kuai (9):
>    blk-throttle: fix that io throttle can only work for single bio
>    blk-throttle: prevent overflow while calculating wait time
>    blk-throttle: factor out code to calculate ios/bytes_allowed
>    blk-throttle: fix io hung due to configuration updates
>    blk-throttle: improve handling of re-entered bio for bps limit
>    blk-throttle: use 'READ/WRITE' instead of '0/1'
>    blk-throttle: calling throtl_dequeue/enqueue_tg in pairs
>    blk-throttle: cleanup tg_update_disptime()
>    blk-throttle: clean up flag 'THROTL_TG_PENDING'
> 
>   block/blk-throttle.c | 175 ++++++++++++++++++++++++++++++-------------
>   block/blk-throttle.h |  20 ++++-
>   2 files changed, 139 insertions(+), 56 deletions(-)
> 

Re: [PATCH v7 0/9] bugfix and cleanup for blk-throttle

[Yu Kuai]

在 2022/08/02 22:04, Yu Kuai 写道:
> From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> 
> Changes in v7:
>   - add patch 5 to improve handling of re-entered bio for bps limit
>   - as suggested by Tejun, add some comments
>   - sdd some Acked tag by Tejun
> Changes in v6:
>   - rename parameter in patch 3
>   - add comments and reviewed tag for patch 4
> Changes in v5:
>   - add comments in patch 4
>   - clear bytes/io_skipped in throtl_start_new_slice_with_credit() in
>   patch 4
>   - and cleanup patches 5-8
> Changes in v4:
>   - add reviewed-by tag for patch 1
>   - add patch 2,3
>   - use a different way to fix io hung in patch 4
> Changes in v3:
>   - fix a check in patch 1
>   - fix link err in patch 2 on 32-bit platform
>   - handle overflow in patch 2
> Changes in v2:
>   - use a new solution suggested by Ming
>   - change the title of patch 1
>   - add patch 2
> 
> Patch 1 fix that blk-throttle can't work if multiple bios are throttle.
> Patch 2 fix overflow while calculating wait time.
> Patch 3,4 fix io hung due to configuration updates.
> Patch 5 improve handling of re-entered bio for bps limit.
> Patch 6-9 are cleanup patches, there are no functional changes, just
> some places that I think can be optimized during code review.
> 
friendly ping ...
> Previous version:
> v1: https://lore.kernel.org/all/20220517134909.2910251-1-yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org/
> v2: https://lore.kernel.org/all/20220518072751.1188163-1-yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org/
> v3: https://lore.kernel.org/all/20220519085811.879097-1-yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org/
> v4: https://lore.kernel.org/all/20220523082633.2324980-1-yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org/
> v5: https://lore.kernel.org/all/20220528064330.3471000-1-yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org/
> v6: https://lore.kernel.org/all/20220701093441.885741-1-yukuai1-XF6JlduFytWkHkcT6e4Xnw@public.gmane.org/
> 
> Yu Kuai (9):
>    blk-throttle: fix that io throttle can only work for single bio
>    blk-throttle: prevent overflow while calculating wait time
>    blk-throttle: factor out code to calculate ios/bytes_allowed
>    blk-throttle: fix io hung due to configuration updates
>    blk-throttle: improve handling of re-entered bio for bps limit
>    blk-throttle: use 'READ/WRITE' instead of '0/1'
>    blk-throttle: calling throtl_dequeue/enqueue_tg in pairs
>    blk-throttle: cleanup tg_update_disptime()
>    blk-throttle: clean up flag 'THROTL_TG_PENDING'
> 
>   block/blk-throttle.c | 175 ++++++++++++++++++++++++++++++-------------
>   block/blk-throttle.h |  20 ++++-
>   2 files changed, 139 insertions(+), 56 deletions(-)
>