From: Yu Kuai <[email protected]>
Currently, "tg->has_rules" and "tg->flags & THROTL_TG_HAS_IOPS_LIMIT"
both try to bypass bios that don't need to be throttled, however, they are
a little redundant and both not perfect:
1) "tg->has_rules" only distinguish read and write, but not iops and bps
limit.
2) "tg->flags & THROTL_TG_HAS_IOPS_LIMIT" only check if iops limit
exist, read and write is not distinguished, and bps limit is not
checked.
With this patchset, biowon't be bypassedd if:
1) Bio is read/write, and corresponding read/write iops limit exist.
2) If corresponding iops limit doesn't exist, corresponding bps limit
exist and bio is not throttled before.
Yu Kuai (2):
blk-throttle: remove THROTL_TG_HAS_IOPS_LIMIT
blk-throttle: improve bypassing bios checkings
block/blk-throttle.c | 21 +++++++--------------
block/blk-throttle.h | 28 +++++++++++++++++++---------
2 files changed, 26 insertions(+), 23 deletions(-)
--
2.31.1
From: Yu Kuai <[email protected]>
"tg->has_rules" is extended to "tg->has_rules_iops/bps", thus bios that
don't need to be throttled can be checked accurately.
With this patch, bio will be throttled if:
1) Bio is read/write, and corresponding read/write iops limit exist.
2) If corresponding doesn't exist, corresponding bps limit exist and
bio is not throttled before.
Signed-off-by: Yu Kuai <[email protected]>
---
block/blk-throttle.c | 13 +++++++++----
block/blk-throttle.h | 22 +++++++++++++++++++---
2 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index a062539d84d0..78316955e30f 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -421,11 +421,16 @@ static void tg_update_has_rules(struct throtl_grp *tg)
struct throtl_data *td = tg->td;
int rw;
- for (rw = READ; rw <= WRITE; rw++)
- tg->has_rules[rw] = (parent_tg && parent_tg->has_rules[rw]) ||
+ for (rw = READ; rw <= WRITE; rw++) {
+ tg->has_rules_iops[rw] =
+ (parent_tg && parent_tg->has_rules_iops[rw]) ||
(td->limit_valid[td->limit_index] &&
- (tg_bps_limit(tg, rw) != U64_MAX ||
- tg_iops_limit(tg, rw) != UINT_MAX));
+ tg_iops_limit(tg, rw) != UINT_MAX);
+ tg->has_rules_bps[rw] =
+ (parent_tg && parent_tg->has_rules_bps[rw]) ||
+ (td->limit_valid[td->limit_index] &&
+ (tg_bps_limit(tg, rw) != U64_MAX));
+ }
}
static void throtl_pd_online(struct blkg_policy_data *pd)
diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index 3994b89dfa11..69f00012d616 100644
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -98,7 +98,8 @@ struct throtl_grp {
unsigned int flags;
/* are there any throtl rules between this group and td? */
- bool has_rules[2];
+ bool has_rules_bps[2];
+ bool has_rules_iops[2];
/* internally used bytes per second rate limits */
uint64_t bps[2][LIMIT_CNT];
@@ -178,11 +179,26 @@ void blk_throtl_exit(struct request_queue *q);
void blk_throtl_register_queue(struct request_queue *q);
bool __blk_throtl_bio(struct bio *bio);
void blk_throtl_cancel_bios(struct request_queue *q);
-static inline bool blk_throtl_bio(struct bio *bio)
+
+static inline bool blk_should_throtl(struct bio *bio)
{
struct throtl_grp *tg = blkg_to_tg(bio->bi_blkg);
+ int rw = bio_data_dir(bio);
+
+ /* iops limit is always counted */
+ if (tg->has_rules_iops[rw])
+ return true;
+
+ if (tg->has_rules_bps[rw] && !bio_flagged(bio, BIO_BPS_THROTTLED))
+ return true;
+
+ return false;
+}
+
+static inline bool blk_throtl_bio(struct bio *bio)
+{
- if (!tg->has_rules[bio_data_dir(bio)])
+ if (!blk_should_throtl(bio))
return false;
return __blk_throtl_bio(bio);
--
2.31.1
From: Yu Kuai <[email protected]>
Currently, "tg->has_rules" and "tg->flags & THROTL_TG_HAS_IOPS_LIMIT"
both try to bypass bios that don't need to be throttled, however, they are
a little redundant and both not perfect:
1) "tg->has_rules" only distinguish read and write, but not iops and bps
limit.
2) "tg->flags & THROTL_TG_HAS_IOPS_LIMIT" only check if iops limit
exist, read and write is not distinguished, and bps limit is not
checked.
tg->has_rules will extended to distinguish bps and iops in the following
patch. There is no need to keep the flag.
Signed-off-by: Yu Kuai <[email protected]>
---
block/blk-throttle.c | 16 ++--------------
block/blk-throttle.h | 8 +-------
2 files changed, 3 insertions(+), 21 deletions(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 55f2d985cfbb..a062539d84d0 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -420,24 +420,12 @@ static void tg_update_has_rules(struct throtl_grp *tg)
struct throtl_grp *parent_tg = sq_to_tg(tg->service_queue.parent_sq);
struct throtl_data *td = tg->td;
int rw;
- int has_iops_limit = 0;
-
- for (rw = READ; rw <= WRITE; rw++) {
- unsigned int iops_limit = tg_iops_limit(tg, rw);
+ for (rw = READ; rw <= WRITE; rw++)
tg->has_rules[rw] = (parent_tg && parent_tg->has_rules[rw]) ||
(td->limit_valid[td->limit_index] &&
(tg_bps_limit(tg, rw) != U64_MAX ||
- iops_limit != UINT_MAX));
-
- if (iops_limit != UINT_MAX)
- has_iops_limit = 1;
- }
-
- if (has_iops_limit)
- tg->flags |= THROTL_TG_HAS_IOPS_LIMIT;
- else
- tg->flags &= ~THROTL_TG_HAS_IOPS_LIMIT;
+ tg_iops_limit(tg, rw) != UINT_MAX));
}
static void throtl_pd_online(struct blkg_policy_data *pd)
diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index 66b4292b1b92..3994b89dfa11 100644
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -55,8 +55,7 @@ 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_CANCELING = 1 << 2, /* starts to cancel bio */
};
enum {
@@ -183,11 +182,6 @@ static inline bool blk_throtl_bio(struct bio *bio)
{
struct throtl_grp *tg = blkg_to_tg(bio->bi_blkg);
- /* no need to throttle bps any more if the bio has been throttled */
- if (bio_flagged(bio, BIO_BPS_THROTTLED) &&
- !(tg->flags & THROTL_TG_HAS_IOPS_LIMIT))
- return false;
-
if (!tg->has_rules[bio_data_dir(bio)])
return false;
--
2.31.1
On Wed, Sep 21, 2022 at 05:53:09PM +0800, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> "tg->has_rules" is extended to "tg->has_rules_iops/bps", thus bios that
> don't need to be throttled can be checked accurately.
>
> With this patch, bio will be throttled if:
>
> 1) Bio is read/write, and corresponding read/write iops limit exist.
> 2) If corresponding doesn't exist, corresponding bps limit exist and
> bio is not throttled before.
>
> Signed-off-by: Yu Kuai <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
On Wed, Sep 21, 2022 at 05:53:08PM +0800, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> Currently, "tg->has_rules" and "tg->flags & THROTL_TG_HAS_IOPS_LIMIT"
> both try to bypass bios that don't need to be throttled, however, they are
> a little redundant and both not perfect:
>
> 1) "tg->has_rules" only distinguish read and write, but not iops and bps
> limit.
> 2) "tg->flags & THROTL_TG_HAS_IOPS_LIMIT" only check if iops limit
> exist, read and write is not distinguished, and bps limit is not
> checked.
>
> tg->has_rules will extended to distinguish bps and iops in the following
> patch. There is no need to keep the flag.
>
> Signed-off-by: Yu Kuai <[email protected]>
Acked-by: Tejun Heo <[email protected]>
> @@ -183,11 +182,6 @@ static inline bool blk_throtl_bio(struct bio *bio)
> {
> struct throtl_grp *tg = blkg_to_tg(bio->bi_blkg);
>
> - /* no need to throttle bps any more if the bio has been throttled */
> - if (bio_flagged(bio, BIO_BPS_THROTTLED) &&
> - !(tg->flags & THROTL_TG_HAS_IOPS_LIMIT))
> - return false;
> -
This temporary removal would break the double accounting until the next
patch, right? That might be worth noting but this looks like an okay way to
go about it.
Thanks.
--
tejun
On Wed, 21 Sep 2022 17:53:07 +0800, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> Currently, "tg->has_rules" and "tg->flags & THROTL_TG_HAS_IOPS_LIMIT"
> both try to bypass bios that don't need to be throttled, however, they are
> a little redundant and both not perfect:
>
> 1) "tg->has_rules" only distinguish read and write, but not iops and bps
> limit.
> 2) "tg->flags & THROTL_TG_HAS_IOPS_LIMIT" only check if iops limit
> exist, read and write is not distinguished, and bps limit is not
> checked.
>
> [...]
Applied, thanks!
[1/2] blk-throttle: remove THROTL_TG_HAS_IOPS_LIMIT
commit: 85496749904016f36b69332f73a1cf3ecfee828f
[2/2] blk-throttle: improve bypassing bios checkings
commit: 81c7a63abc7c0be572b4f853e913ce93a34f6e1b
Best regards,
--
Jens Axboe