[PATCH] block-throttle: avoid double charge

[PATCH] block-throttle: avoid double charge

[Shaohua Li]

If a bio is throttled and splitted after throttling, the bio could be
resubmited and enters the throttling again. This will cause part of the
bio is charged multiple times. If the cgroup has an IO limit, the double
charge will significantly harm the performance. The bio split becomes
quite common after arbitrary bio size change.

To fix this, we record the disk info a bio is throttled against. If a
bio is throttled and issued, we record the info. We copy the info to
cloned bio, so cloned bio (including splitted bio) will not be throttled
again. Stacked block device driver will change cloned bio's bi_disk, if
a bio's bi_disk is changed, the recorded throttle disk info is invalid,
we should throttle again. That's the reason why we can't use a single
bit to indicate if a cloned bio should be throttled.

We only record gendisk here, if a cloned bio is remapped to other disk,
it's very unlikely only partno is changed.

Some sort of this patch probably should go into stable since v4.2

Cc: Tejun Heo <tj@kernel.org>
Cc: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 block/bio.c               |  3 +++
 block/blk-throttle.c      | 15 ++++++++++++---
 include/linux/blk_types.h |  4 ++++
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 8338304..dce8314 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -597,6 +597,9 @@ void __bio_clone_fast(struct bio *bio, struct bio *bio_src)
 	 * so we don't set nor calculate new physical/hw segment counts here
 	 */
 	bio->bi_disk = bio_src->bi_disk;
+#ifdef CONFIG_BLK_DEV_THROTTLING
+	bio->bi_throttled_disk = bio_src->bi_throttled_disk;
+#endif
 	bio_set_flag(bio, BIO_CLONED);
 	bio->bi_opf = bio_src->bi_opf;
 	bio->bi_write_hint = bio_src->bi_write_hint;
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index ee6d7b0..155549a 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -2130,9 +2130,15 @@ bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg,
 
 	WARN_ON_ONCE(!rcu_read_lock_held());
 
-	/* see throtl_charge_bio() */
-	if (bio_flagged(bio, BIO_THROTTLED) || !tg->has_rules[rw])
+	/*
+	 * see throtl_charge_bio() for BIO_THROTTLED. If a bio is throttled
+	 * against a disk but remapped to other disk, we should throttle it
+	 * again
+	 */
+	if (bio_flagged(bio, BIO_THROTTLED) || !tg->has_rules[rw] ||
+	    (bio->bi_throttled_disk && bio->bi_throttled_disk == bio->bi_disk))
 		goto out;
+	bio->bi_throttled_disk = NULL;
 
 	spin_lock_irq(q->queue_lock);
 
@@ -2227,8 +2233,11 @@ bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg,
 	 * don't want bios to leave with the flag set.  Clear the flag if
 	 * being issued.
 	 */
-	if (!throttled)
+	if (!throttled) {
 		bio_clear_flag(bio, BIO_THROTTLED);
+		/* if the bio is cloned, we don't throttle it again */
+		bio->bi_throttled_disk = bio->bi_disk;
+	}
 
 #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
 	if (throttled || !td->track_bio_latency)
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 3385c89..2507566 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -89,6 +89,10 @@ struct bio {
 	void			*bi_cg_private;
 	struct blk_issue_stat	bi_issue_stat;
 #endif
+#ifdef CONFIG_BLK_DEV_THROTTLING
+	/* record which disk the bio is throttled against */
+	struct gendisk		*bi_throttled_disk;
+#endif
 #endif
 	union {
 #if defined(CONFIG_BLK_DEV_INTEGRITY)
-- 
2.9.5

Re: [PATCH] block-throttle: avoid double charge

[Tejun Heo]

Hello, Shaohua.

On Fri, Oct 13, 2017 at 11:10:29AM -0700, Shaohua Li wrote:
> If a bio is throttled and splitted after throttling, the bio could be
> resubmited and enters the throttling again. This will cause part of the
> bio is charged multiple times. If the cgroup has an IO limit, the double
> charge will significantly harm the performance. The bio split becomes
> quite common after arbitrary bio size change.

Missed the patch previously.  Sorry about that.

> Some sort of this patch probably should go into stable since v4.2

Seriously.

> @@ -2130,9 +2130,15 @@ bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg,
>  
>  	WARN_ON_ONCE(!rcu_read_lock_held());
>  
> -	/* see throtl_charge_bio() */
> -	if (bio_flagged(bio, BIO_THROTTLED) || !tg->has_rules[rw])
> +	/*
> +	 * see throtl_charge_bio() for BIO_THROTTLED. If a bio is throttled
> +	 * against a disk but remapped to other disk, we should throttle it
> +	 * again
> +	 */
> +	if (bio_flagged(bio, BIO_THROTTLED) || !tg->has_rules[rw] ||
> +	    (bio->bi_throttled_disk && bio->bi_throttled_disk == bio->bi_disk))
>  		goto out;
> +	bio->bi_throttled_disk = NULL;

So, one question I have is whether we need both BIO_THROTTLED and
bi_throttled_disk.  Can't we replace BIO_THROTTLED w/
bi_throttled_disk?

Thanks.

-- 
tejun

Re: [PATCH] block-throttle: avoid double charge

[Tejun Heo]

On Mon, Nov 13, 2017 at 12:03:38PM -0800, Tejun Heo wrote:
> So, one question I have is whether we need both BIO_THROTTLED and
> bi_throttled_disk.  Can't we replace BIO_THROTTLED w/
> bi_throttled_disk?

IOW, won't something like the following work?  (not tested yet)

Thanks.

---
 block/bio.c               |    3 +++
 block/blk-throttle.c      |   20 +++++++-------------
 include/linux/blk_types.h |    4 ++++
 3 files changed, 14 insertions(+), 13 deletions(-)

--- a/block/bio.c
+++ b/block/bio.c
@@ -597,6 +597,9 @@ void __bio_clone_fast(struct bio *bio, s
 	 * so we don't set nor calculate new physical/hw segment counts here
 	 */
 	bio->bi_disk = bio_src->bi_disk;
+#ifdef CONFIG_BLK_DEV_THROTTLING
+	bio->bi_throttled_disk = bio_src->bi_throttled_disk;
+#endif
 	bio_set_flag(bio, BIO_CLONED);
 	bio->bi_opf = bio_src->bi_opf;
 	bio->bi_write_hint = bio_src->bi_write_hint;
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1051,13 +1051,12 @@ static void throtl_charge_bio(struct thr
 	tg->last_io_disp[rw]++;
 
 	/*
-	 * BIO_THROTTLED is used to prevent the same bio to be throttled
+	 * bi_throttled_disk is used to prevent the same bio to be throttled
 	 * more than once as a throttled bio will go through blk-throtl the
 	 * second time when it eventually gets issued.  Set it when a bio
 	 * is being charged to a tg.
 	 */
-	if (!bio_flagged(bio, BIO_THROTTLED))
-		bio_set_flag(bio, BIO_THROTTLED);
+	bio->bi_throttled_disk = bio->bi_disk;
 }
 
 /**
@@ -2131,8 +2130,11 @@ bool blk_throtl_bio(struct request_queue
 
 	WARN_ON_ONCE(!rcu_read_lock_held());
 
-	/* see throtl_charge_bio() */
-	if (bio_flagged(bio, BIO_THROTTLED) || !tg->has_rules[rw])
+	/*
+	 * See throtl_charge_bio().  If a bio is throttled against a disk
+	 * but remapped to other disk, we should throttle it again
+	 */
+	if (bio->bi_throttled_disk == bio->bi_disk || !tg->has_rules[rw])
 		goto out;
 
 	spin_lock_irq(q->queue_lock);
@@ -2223,14 +2225,6 @@ again:
 out_unlock:
 	spin_unlock_irq(q->queue_lock);
 out:
-	/*
-	 * As multiple blk-throtls may stack in the same issue path, we
-	 * don't want bios to leave with the flag set.  Clear the flag if
-	 * being issued.
-	 */
-	if (!throttled)
-		bio_clear_flag(bio, BIO_THROTTLED);
-
 #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
 	if (throttled || !td->track_bio_latency)
 		bio->bi_issue_stat.stat |= SKIP_LATENCY;
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -90,6 +90,10 @@ struct bio {
 	void			*bi_cg_private;
 	struct blk_issue_stat	bi_issue_stat;
 #endif
+#ifdef CONFIG_BLK_DEV_THROTTLING
+	/* record which disk the bio is throttled against */
+	struct gendisk		*bi_throttled_disk;
+#endif
 #endif
 	union {
 #if defined(CONFIG_BLK_DEV_INTEGRITY)