Skip to contentSkip to conversation

Some improvements for blk throttle

PATCH Superseded Baolin Wang 1 participant 6 messages Sep 20, 2020 – Sep 29, 2020

Subsystems (3)
Block layer cgroup cgroup blkio

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Baolin Wang

author
#0 lore raw

cover Some improvements for blk throttle

Hi,

This patch set did some improvements for blk throttle, please help to review. Thanks.

Baolin Wang (4):
blk-throttle: Remove a meaningless parameter for
throtl_downgrade_state()
blk-throttle: Avoid getting the current time if tg->last_finish_time
is 0
blk-throttle: Avoid tracking latency if low limit is invalid
blk-throttle: Fix IO hang for a corner case

 block/blk-throttle.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)
Signature · 1 line
1.8.3.1
author
#1 lore raw

PATCH 1/4 blk-throttle: Remove a meaningless parameter for throtl_downgrade_state()

The throtl_downgrade_state() is always used to change to LIMIT_LOW limitation, thus remove the latter meaningless parameter which indicates the limitation index.

Signed-off-by Baolin Wang <[email protected]>
---
 block/blk-throttle.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
block/blk-throttle.c +3 -3
index 36ba61c..4007b26 100644
@@ -1970,7 +1970,7 @@ static void throtl_upgrade_state(struct throtl_data *td)
 	queue_work(kthrotld_workqueue, &td->dispatch_work);
 }
 
-static void throtl_downgrade_state(struct throtl_data *td, int new)
+static void throtl_downgrade_state(struct throtl_data *td)
 {
 	td->scale /= 2;
 

@@ -1980,7 +1980,7 @@ static void throtl_downgrade_state(struct throtl_data *td, int new)
 		return;
 	}
 
-	td->limit_index = new;
+	td->limit_index = LIMIT_LOW;
 	td->low_downgrade_time = jiffies;
 }
 

@@ -2067,7 +2067,7 @@ static void throtl_downgrade_check(struct throtl_grp *tg)
 	 * cgroups
 	 */
 	if (throtl_hierarchy_can_downgrade(tg))
-		throtl_downgrade_state(tg->td, LIMIT_LOW);
+		throtl_downgrade_state(tg->td);
 
 	tg->last_bytes_disp[READ] = 0;
 	tg->last_bytes_disp[WRITE] = 0;
Signature · 1 line
1.8.3.1
author
#2 lore raw

PATCH 2/4 blk-throttle: Avoid getting the current time if tg->last_finish_time is 0

In reply to Baolin Wang

We only update the tg->last_finish_time when the low limitaion is enabled, so we can move the tg->last_finish_time validation a little forward to avoid getting the unnecessary current time stamp if the the low limitation is not enabled.

Signed-off-by Baolin Wang <[email protected]>
---
 block/blk-throttle.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
block/blk-throttle.c +6 -2
index 4007b26..7e72102 100644
@@ -2077,10 +2077,14 @@ static void throtl_downgrade_check(struct throtl_grp *tg)
 
 static void blk_throtl_update_idletime(struct throtl_grp *tg)
 {
-	unsigned long now = ktime_get_ns() >> 10;
+	unsigned long now;
 	unsigned long last_finish_time = tg->last_finish_time;
 
-	if (now <= last_finish_time || last_finish_time == 0 ||
+	if (last_finish_time == 0)
+		return;
+
+	now = ktime_get_ns() >> 10;
+	if (now <= last_finish_time ||
 	    last_finish_time == tg->checked_last_finish_time)
 		return;
Signature · 1 line
1.8.3.1
author
#3 lore raw

PATCH 3/4 blk-throttle: Avoid tracking latency if low limit is invalid

In reply to Baolin Wang

The IO latency tracking is only for LOW limit, so we should add a validation to avoid redundant latency tracking if the LOW limit is not valid.

Signed-off-by Baolin Wang <baolin.wang-KPsoFbNs7GizrGE5bRqYAgC/[email protected]>
---
 block/blk-throttle.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
block/blk-throttle.c +3 -1
index 7e72102..b0d8f7c 100644
@@ -2100,7 +2100,7 @@ static void throtl_update_latency_buckets(struct throtl_data *td)
 	unsigned long last_latency[2] = { 0 };
 	unsigned long latency[2];
 
-	if (!blk_queue_nonrot(td->queue))
+	if (!blk_queue_nonrot(td->queue) || !td->limit_valid[LIMIT_LOW])
 		return;
 	if (time_before(jiffies, td->last_calculate_time + HZ))
 		return;

@@ -2338,6 +2338,8 @@ void blk_throtl_bio_endio(struct bio *bio)
 	if (!blkg)
 		return;
 	tg = blkg_to_tg(blkg);
+	if (!tg->td->limit_valid[LIMIT_LOW])
+		return;
 
 	finish_time_ns = ktime_get_ns();
 	tg->last_finish_time = finish_time_ns >> 10;
Signature · 1 line
1.8.3.1
author
#4 lore raw

PATCH 4/4 blk-throttle: Fix IO hang for a corner case

It can not scale up in throtl_adjusted_limit() if we set bps or iops is 1, which will cause IO hang when enable low limit. Thus we should treat 1 as a illegal value to avoid this issue.

Signed-off-by Baolin Wang <[email protected]>
---
 block/blk-throttle.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
block/blk-throttle.c +4 -4
index b0d8f7c..0649bce 100644
@@ -1687,13 +1687,13 @@ static ssize_t tg_set_limit(struct kernfs_open_file *of,
 			goto out_finish;
 
 		ret = -EINVAL;
-		if (!strcmp(tok, "rbps"))
+		if (!strcmp(tok, "rbps") && val > 1)
 			v[0] = val;
-		else if (!strcmp(tok, "wbps"))
+		else if (!strcmp(tok, "wbps") && val > 1)
 			v[1] = val;
-		else if (!strcmp(tok, "riops"))
+		else if (!strcmp(tok, "riops") && val > 1)
 			v[2] = min_t(u64, val, UINT_MAX);
-		else if (!strcmp(tok, "wiops"))
+		else if (!strcmp(tok, "wiops") && val > 1)
 			v[3] = min_t(u64, val, UINT_MAX);
 		else if (off == LIMIT_LOW && !strcmp(tok, "idle"))
 			idle_time = val;
Signature · 1 line
1.8.3.1
author
#5 lore raw
In reply to Baolin Wang

Hi Jens,

Quoted 4 lines

Hi,

This patch set did some improvements for blk throttle, please help to review. Thanks.

Do you have any comments for this patch set? Thanks.

Quoted 12 lines

Baolin Wang (4):
blk-throttle: Remove a meaningless parameter for
throtl_downgrade_state()
blk-throttle: Avoid getting the current time if tg->last_finish_time
is 0
blk-throttle: Avoid tracking latency if low limit is invalid
blk-throttle: Fix IO hang for a corner case

  block/blk-throttle.c | 26 ++++++++++++++++----------
  1 file changed, 16 insertions(+), 10 deletions(-)