Commit 320fb0f9 authored by Yu Kuai's avatar Yu Kuai Committed by Jens Axboe
Browse files

blk-throttle: fix that io throttle can only work for single bio 



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 flagged:

__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) flagged 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 9f5ede3c ("block: throttle split bio in case of iops limit")
support to count split bios for iops limit, thus it adds flagged bio
checking in tg_with_in_bps_limit() so that split 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, handle iops/bps limit in different ways:

1) for iops limit, there is no flag to record if the bio is throttled,
   and iops is always applied.
2) for bps limit, original bio will be flagged with BIO_BPS_THROTTLED,
   and io throttle will ignore bio with the flag.

Noted this patch also remove the code to set flag in __bio_clone(), it's
introduced in commit 111be883 ("block-throttle: avoid double
charge"), and author thinks split bio can be resubmited and throttled
again, which is wrong because split bio will continue to dispatch from
caller.

Fixes: 9f5ede3c ("block: throttle split bio in case of iops limit")
Cc: <stable@vger.kernel.org>
Signed-off-by: default avatarYu Kuai <yukuai3@huawei.com>
Acked-by: default avatarTejun Heo <tj@kernel.org>
Link: https://lore.kernel.org/r/20220829022240.3348319-2-yukuai1@huaweicloud.com


Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
parent 4acb8341

block/bio.c

+0 −2
Original line number Diff line number Diff line
@@ -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;

block/blk-throttle.c

+6 −14
Original line number Diff line number Diff line
@@ -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 || bio_flagged(bio, BIO_BPS_THROTTLED)) {

		if (wait)

			*wait = 0;

		return true;
@@ -921,22 +921,13 @@ 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)) {

	if (!bio_flagged(bio, BIO_BPS_THROTTLED)) {

		tg->bytes_disp[rw] += bio_size;

		tg->last_bytes_disp[rw] += bio_size;

	}



	tg->io_disp[rw]++;

	tg->last_io_disp[rw]++;



	/*

	 * BIO_THROTTLED 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);

}



/**
@@ -1026,6 +1017,7 @@ static void tg_dispatch_one_bio(struct throtl_grp *tg, bool rw) 
	sq->nr_queued[rw]--;



	throtl_charge_bio(tg, bio);

	bio_set_flag(bio, BIO_BPS_THROTTLED);



	/*

	 * If our parent is another tg, we just need to transfer @bio to
@@ -2181,9 +2173,11 @@ bool __blk_throtl_bio(struct bio *bio) 
		qn = &tg->qnode_on_parent[rw];

		sq = sq->parent_sq;

		tg = sq_to_tg(sq);

		if (!tg)

		if (!tg) {

			bio_set_flag(bio, BIO_BPS_THROTTLED);

			goto out_unlock;

		}

	}



	/* out-of-limit, queue to @tg */

	throtl_log(sq, "[%c] bio. bdisp=%llu sz=%u bps=%llu iodisp=%u iops=%u queued=%d/%d",
@@ -2211,8 +2205,6 @@ bool __blk_throtl_bio(struct bio *bio) 
	}



out_unlock:

	bio_set_flag(bio, BIO_THROTTLED);



#ifdef CONFIG_BLK_DEV_THROTTLING_LOW

	if (throttled || !td->track_bio_latency)

		bio->bi_issue.value |= BIO_ISSUE_THROTL_SKIP_LATENCY;

block/blk-throttle.h

+1 −1
Original line number Diff line number Diff line
@@ -175,7 +175,7 @@ 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_THROTTLED) &&

	if (bio_flagged(bio, BIO_BPS_THROTTLED) &&

	    !(tg->flags & THROTL_TG_HAS_IOPS_LIMIT))

		return false;

include/linux/bio.h

+1 −1
Original line number Diff line number Diff line
@@ -509,7 +509,7 @@ static inline void bio_set_dev(struct bio *bio, struct block_device *bdev) 
{

	bio_clear_flag(bio, BIO_REMAPPED);

	if (bio->bi_bdev != bdev)

		bio_clear_flag(bio, BIO_THROTTLED);

		bio_clear_flag(bio, BIO_BPS_THROTTLED);

	bio->bi_bdev = bdev;

	bio_associate_blkg(bio);

}

include/linux/blk_types.h

+1 −1
Original line number Diff line number Diff line
@@ -325,7 +325,7 @@ enum { 
	BIO_QUIET,		/* Make BIO Quiet */

	BIO_CHAIN,		/* chained bio, ->bi_remaining in effect */

	BIO_REFFED,		/* bio has elevated ->bi_cnt */

	BIO_THROTTLED,		/* This bio has already been subjected to

	BIO_BPS_THROTTLED,	/* This bio has already been subjected to

				 * throttling rules. Don't do it again. */

	BIO_TRACE_COMPLETION,	/* bio_endio() should trace the final completion

				 * of this bio. */