* [PATCH] block: throttle: charge io re-submission for iops limit
@ 2021-12-30 3:45 Ming Lei
2022-01-06 2:41 ` Ming Lei
2022-01-06 7:46 ` brookxu
0 siblings, 2 replies; 6+ messages in thread
From: Ming Lei @ 2021-12-30 3:45 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Ming Lei, lining2020x, Tejun Heo, Chunguang Xu
Commit 111be8839817 ("block-throttle: avoid double charge") marks bio as
BIO_THROTTLED unconditionally if __blk_throtl_bio() is called on this bio,
then this bio won't be called into __blk_throtl_bio() any more. This way
is to avoid double charge in case of bio splitting. It is reasonable for
read/write throughput limit, but not reasonable for IOPS limit because
block layer provides io accounting against split bio.
Chunguang Xu has already observed this issue and fixed it in commit
4f1e9630afe6 ("blk-throtl: optimize IOPS throttle for large IO scenarios").
However, that patch only covers bio splitting in __blk_queue_split(), and
we have other kind of bio splitting, such as bio_split() & submit_bio_noacct()
and other ways.
This patch tries to fix the issue in one generic way, by always charge
the bio for iops limit in blk_throtl_bio() in case that BIO_THROTTLED
is set. This way is reasonable: re-submission & fast-cloned bio is charged
if it is submitted to same disk/queue, and BIO_THROTTLED will be cleared
if bio->bi_bdev is changed.
Reported-by: lining2020x@163.com
Cc: Tejun Heo <tj@kernel.org>
Cc: Chunguang Xu <brookxu@tencent.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
block/blk-merge.c | 2 --
block/blk-throttle.c | 2 +-
block/blk-throttle.h | 8 +++++---
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 4de34a332c9f..f5255991b773 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -368,8 +368,6 @@ void __blk_queue_split(struct request_queue *q, struct bio **bio,
trace_block_split(split, (*bio)->bi_iter.bi_sector);
submit_bio_noacct(*bio);
*bio = split;
-
- blk_throtl_charge_bio_split(*bio);
}
}
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 7c462c006b26..ea532c178385 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -2043,7 +2043,7 @@ static inline void throtl_update_latency_buckets(struct throtl_data *td)
}
#endif
-void blk_throtl_charge_bio_split(struct bio *bio)
+void blk_throtl_charge_for_iops_limit(struct bio *bio)
{
struct blkcg_gq *blkg = bio->bi_blkg;
struct throtl_grp *parent = blkg_to_tg(blkg);
diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index 175f03abd9e4..954b9cac19b7 100644
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -158,20 +158,22 @@ static inline struct throtl_grp *blkg_to_tg(struct blkcg_gq *blkg)
static inline int blk_throtl_init(struct request_queue *q) { return 0; }
static inline void blk_throtl_exit(struct request_queue *q) { }
static inline void blk_throtl_register_queue(struct request_queue *q) { }
-static inline void blk_throtl_charge_bio_split(struct bio *bio) { }
+static inline void blk_throtl_charge_for_iops_limit(struct bio *bio) { }
static inline bool blk_throtl_bio(struct bio *bio) { return false; }
#else /* CONFIG_BLK_DEV_THROTTLING */
int blk_throtl_init(struct request_queue *q);
void blk_throtl_exit(struct request_queue *q);
void blk_throtl_register_queue(struct request_queue *q);
-void blk_throtl_charge_bio_split(struct bio *bio);
+void blk_throtl_charge_for_iops_limit(struct bio *bio);
bool __blk_throtl_bio(struct bio *bio);
static inline bool blk_throtl_bio(struct bio *bio)
{
struct throtl_grp *tg = blkg_to_tg(bio->bi_blkg);
- if (bio_flagged(bio, BIO_THROTTLED))
+ if (bio_flagged(bio, BIO_THROTTLED)) {
+ blk_throtl_charge_for_iops_limit(bio);
return false;
+ }
if (!tg->has_rules[bio_data_dir(bio)])
return false;
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] block: throttle: charge io re-submission for iops limit
2021-12-30 3:45 [PATCH] block: throttle: charge io re-submission for iops limit Ming Lei
@ 2022-01-06 2:41 ` Ming Lei
2022-01-06 7:46 ` brookxu
1 sibling, 0 replies; 6+ messages in thread
From: Ming Lei @ 2022-01-06 2:41 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, lining2020x, Tejun Heo, Chunguang Xu
On Thu, Dec 30, 2021 at 11:45:13AM +0800, Ming Lei wrote:
> Commit 111be8839817 ("block-throttle: avoid double charge") marks bio as
> BIO_THROTTLED unconditionally if __blk_throtl_bio() is called on this bio,
> then this bio won't be called into __blk_throtl_bio() any more. This way
> is to avoid double charge in case of bio splitting. It is reasonable for
> read/write throughput limit, but not reasonable for IOPS limit because
> block layer provides io accounting against split bio.
>
> Chunguang Xu has already observed this issue and fixed it in commit
> 4f1e9630afe6 ("blk-throtl: optimize IOPS throttle for large IO scenarios").
> However, that patch only covers bio splitting in __blk_queue_split(), and
> we have other kind of bio splitting, such as bio_split() & submit_bio_noacct()
> and other ways.
>
> This patch tries to fix the issue in one generic way, by always charge
> the bio for iops limit in blk_throtl_bio() in case that BIO_THROTTLED
> is set. This way is reasonable: re-submission & fast-cloned bio is charged
> if it is submitted to same disk/queue, and BIO_THROTTLED will be cleared
> if bio->bi_bdev is changed.
>
> Reported-by: lining2020x@163.com
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Chunguang Xu <brookxu@tencent.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> block/blk-merge.c | 2 --
> block/blk-throttle.c | 2 +-
> block/blk-throttle.h | 8 +++++---
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 4de34a332c9f..f5255991b773 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -368,8 +368,6 @@ void __blk_queue_split(struct request_queue *q, struct bio **bio,
> trace_block_split(split, (*bio)->bi_iter.bi_sector);
> submit_bio_noacct(*bio);
> *bio = split;
> -
> - blk_throtl_charge_bio_split(*bio);
> }
> }
>
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 7c462c006b26..ea532c178385 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -2043,7 +2043,7 @@ static inline void throtl_update_latency_buckets(struct throtl_data *td)
> }
> #endif
>
> -void blk_throtl_charge_bio_split(struct bio *bio)
> +void blk_throtl_charge_for_iops_limit(struct bio *bio)
> {
> struct blkcg_gq *blkg = bio->bi_blkg;
> struct throtl_grp *parent = blkg_to_tg(blkg);
> diff --git a/block/blk-throttle.h b/block/blk-throttle.h
> index 175f03abd9e4..954b9cac19b7 100644
> --- a/block/blk-throttle.h
> +++ b/block/blk-throttle.h
> @@ -158,20 +158,22 @@ static inline struct throtl_grp *blkg_to_tg(struct blkcg_gq *blkg)
> static inline int blk_throtl_init(struct request_queue *q) { return 0; }
> static inline void blk_throtl_exit(struct request_queue *q) { }
> static inline void blk_throtl_register_queue(struct request_queue *q) { }
> -static inline void blk_throtl_charge_bio_split(struct bio *bio) { }
> +static inline void blk_throtl_charge_for_iops_limit(struct bio *bio) { }
> static inline bool blk_throtl_bio(struct bio *bio) { return false; }
> #else /* CONFIG_BLK_DEV_THROTTLING */
> int blk_throtl_init(struct request_queue *q);
> void blk_throtl_exit(struct request_queue *q);
> void blk_throtl_register_queue(struct request_queue *q);
> -void blk_throtl_charge_bio_split(struct bio *bio);
> +void blk_throtl_charge_for_iops_limit(struct bio *bio);
> bool __blk_throtl_bio(struct bio *bio);
> static inline bool blk_throtl_bio(struct bio *bio)
> {
> struct throtl_grp *tg = blkg_to_tg(bio->bi_blkg);
>
> - if (bio_flagged(bio, BIO_THROTTLED))
> + if (bio_flagged(bio, BIO_THROTTLED)) {
> + blk_throtl_charge_for_iops_limit(bio);
This way may cause double charge since the bio's iops limit
charge can be done via blk_throtl_dispatch_work_fn() too.
Will post another version for fix this issue.
Thanks,
Ming
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] block: throttle: charge io re-submission for iops limit
2021-12-30 3:45 [PATCH] block: throttle: charge io re-submission for iops limit Ming Lei
2022-01-06 2:41 ` Ming Lei
@ 2022-01-06 7:46 ` brookxu
2022-01-07 3:52 ` Ming Lei
1 sibling, 1 reply; 6+ messages in thread
From: brookxu @ 2022-01-06 7:46 UTC (permalink / raw)
To: Ming Lei, Jens Axboe; +Cc: linux-block, lining2020x, Tejun Heo, Chunguang Xu
Hi Ming:
I think it may be due to other reasons, I test this patch seems work fine,
Can you verify it in your environment?
From 2c7305042e71d0f53ca50a8a3f2eebe6a2dcdb86 Mon Sep 17 00:00:00 2001
From: Chunguang Xu <brookxu@tencent.com>
Date: Thu, 6 Jan 2022 15:18:50 +0800
Subject: [PATCH] blk-throtl: avoid double charge of bio IOPS due to split
After commit 900e08075202("block: move queue enter logic into
blk_mq_submit_bio()"), submit_bio_checks() is moved from the
entrance of the IO stack to the specific submit_bio() entrance.
Therefore, the IO may be splited before entering blk-throtl, so
we need to check whether the BIO is throttled, and only need
to update the io_split_cnt for the throttled bio to avoid
double charge.
Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
block/blk-throttle.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 39bb6e6..2b12fc7 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -2049,6 +2049,9 @@ void blk_throtl_charge_bio_split(struct bio *bio)
struct throtl_service_queue *parent_sq;
bool rw = bio_data_dir(bio);
+ if (!bio_flagged(bio, BIO_THROTTLED))
+ return;
+
do {
if (!parent->has_rules[rw])
break;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] block: throttle: charge io re-submission for iops limit
2022-01-06 7:46 ` brookxu
@ 2022-01-07 3:52 ` Ming Lei
2022-01-07 6:50 ` brookxu
0 siblings, 1 reply; 6+ messages in thread
From: Ming Lei @ 2022-01-07 3:52 UTC (permalink / raw)
To: brookxu; +Cc: Jens Axboe, linux-block, lining2020x, Tejun Heo, Chunguang Xu
On Thu, Jan 06, 2022 at 03:46:54PM +0800, brookxu wrote:
> Hi Ming:
>
> I think it may be due to other reasons, I test this patch seems work fine,
> Can you verify it in your environment?
Your patch can't cover the issue Ning Li reported.
>
>
> From 2c7305042e71d0f53ca50a8a3f2eebe6a2dcdb86 Mon Sep 17 00:00:00 2001
> From: Chunguang Xu <brookxu@tencent.com>
> Date: Thu, 6 Jan 2022 15:18:50 +0800
> Subject: [PATCH] blk-throtl: avoid double charge of bio IOPS due to split
>
> After commit 900e08075202("block: move queue enter logic into
> blk_mq_submit_bio()"), submit_bio_checks() is moved from the
> entrance of the IO stack to the specific submit_bio() entrance.
> Therefore, the IO may be splited before entering blk-throtl, so
> we need to check whether the BIO is throttled, and only need
> to update the io_split_cnt for the throttled bio to avoid
> double charge.
Actually since commit 900e08075202, your commit 4f1e9630afe6
("blk-throtl: optimize IOPS throttle for large IO scenarios") doesn't
need any more, because split bio is always sent to submit_bio_checks().
But I don't think that way is reasonable, especially precise driver tag
and request is consumed by each throttling, so the following patch is posted:
https://lore.kernel.org/linux-block/20220104134223.590803-1-ming.lei@redhat.com/T/#u
Thanks,
Ming
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] block: throttle: charge io re-submission for iops limit
2022-01-07 3:52 ` Ming Lei
@ 2022-01-07 6:50 ` brookxu
2022-01-07 7:35 ` brookxu
0 siblings, 1 reply; 6+ messages in thread
From: brookxu @ 2022-01-07 6:50 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block, lining2020x, Tejun Heo, Chunguang Xu
Ming Lei wrote on 2022/1/7 11:52:
> On Thu, Jan 06, 2022 at 03:46:54PM +0800, brookxu wrote:
>> Hi Ming:
>>
>> I think it may be due to other reasons, I test this patch seems work fine,
>> Can you verify it in your environment?
>
> Your patch can't cover the issue Ning Li reported.
>
>>
>>
>> From 2c7305042e71d0f53ca50a8a3f2eebe6a2dcdb86 Mon Sep 17 00:00:00 2001
>> From: Chunguang Xu <brookxu@tencent.com>
>> Date: Thu, 6 Jan 2022 15:18:50 +0800
>> Subject: [PATCH] blk-throtl: avoid double charge of bio IOPS due to split
>>
>> After commit 900e08075202("block: move queue enter logic into
>> blk_mq_submit_bio()"), submit_bio_checks() is moved from the
>> entrance of the IO stack to the specific submit_bio() entrance.
>> Therefore, the IO may be splited before entering blk-throtl, so
>> we need to check whether the BIO is throttled, and only need
>> to update the io_split_cnt for the throttled bio to avoid
>> double charge.
>
> Actually since commit 900e08075202, your commit 4f1e9630afe6
> ("blk-throtl: optimize IOPS throttle for large IO scenarios") doesn't
> need any more, because split bio is always sent to submit_bio_checks().
This patch should still be necessary for those devices whose IO needs to go
through __submit_bio_fops(). If the IO is split after submit_bio_checks(),
the cloned IO will be marked with BIO_THROTTLED and will not be charged again
when resubmitted.
> But I don't think that way is reasonable, especially precise driver tag
> and request is consumed by each throttling, so the following patch is posted:
Right.
>
> https://lore.kernel.org/linux-block/20220104134223.590803-1-ming.lei@redhat.com/T/#u
>
>
> Thanks,
> Ming
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] block: throttle: charge io re-submission for iops limit
2022-01-07 6:50 ` brookxu
@ 2022-01-07 7:35 ` brookxu
0 siblings, 0 replies; 6+ messages in thread
From: brookxu @ 2022-01-07 7:35 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block, lining2020x, Tejun Heo, Chunguang Xu
brookxu wrote on 2022/1/7 14:50:
>
>
> Ming Lei wrote on 2022/1/7 11:52:
>> On Thu, Jan 06, 2022 at 03:46:54PM +0800, brookxu wrote:
>>> Hi Ming:
>>>
>>> I think it may be due to other reasons, I test this patch seems work fine,
>>> Can you verify it in your environment?
>>
>> Your patch can't cover the issue Ning Li reported.
>>
>>>
>>>
>>> From 2c7305042e71d0f53ca50a8a3f2eebe6a2dcdb86 Mon Sep 17 00:00:00 2001
>>> From: Chunguang Xu <brookxu@tencent.com>
>>> Date: Thu, 6 Jan 2022 15:18:50 +0800
>>> Subject: [PATCH] blk-throtl: avoid double charge of bio IOPS due to split
>>>
>>> After commit 900e08075202("block: move queue enter logic into
>>> blk_mq_submit_bio()"), submit_bio_checks() is moved from the
>>> entrance of the IO stack to the specific submit_bio() entrance.
>>> Therefore, the IO may be splited before entering blk-throtl, so
>>> we need to check whether the BIO is throttled, and only need
>>> to update the io_split_cnt for the throttled bio to avoid
>>> double charge.
>>
>> Actually since commit 900e08075202, your commit 4f1e9630afe6
>> ("blk-throtl: optimize IOPS throttle for large IO scenarios") doesn't
>> need any more, because split bio is always sent to submit_bio_checks().
> This patch should still be necessary for those devices whose IO needs to go
> through __submit_bio_fops(). If the IO is split after submit_bio_checks(),
> the cloned IO will be marked with BIO_THROTTLED and will not be charged again
> when resubmitted.
Excuse me. Think deeply, we should sitll need to add the following judgment?
This maybe more reliable, because we only need to add the IOPS count for
THROTTLED IO that was missed due to split. It can also avoid double charge
problems in the future. Any comments is required.
@@ -2049,6 +2049,9 @@ void blk_throtl_charge_bio_split(struct bio *bio)
struct throtl_service_queue *parent_sq;
bool rw = bio_data_dir(bio);
+ if (!bio_flagged(bio, BIO_THROTTLED))
+ return;
+
do {
if (!parent->has_rules[rw])
break;
>> But I don't think that way is reasonable, especially precise driver tag
>> and request is consumed by each throttling, so the following patch is posted:
>
> Right.
>
>>
>> https://lore.kernel.org/linux-block/20220104134223.590803-1-ming.lei@redhat.com/T/#u
>>
>>
>> Thanks,
>> Ming
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-07 7:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-12-30 3:45 [PATCH] block: throttle: charge io re-submission for iops limit Ming Lei
2022-01-06 2:41 ` Ming Lei
2022-01-06 7:46 ` brookxu
2022-01-07 3:52 ` Ming Lei
2022-01-07 6:50 ` brookxu
2022-01-07 7:35 ` brookxu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).