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 9f5ede3c01f9 ("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 111be8839817 ("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: 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
Cc: <stable@vger.kernel.org>
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Acked-by: Tejun Heo <tj@kernel.org>
Link: https://lore.kernel.org/r/20220829022240.3348319-2-yukuai1@huaweicloud.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/block/bio.c b/block/bio.c
index d3154d8beed725212f896015cb13aaa63d65120a..c88459a9507d6c09454e744ce8a8d69c0a28717b 100644 (file)
--- a/block/bio.c
+++ b/block/bio.c
@@ -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;
 

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 667b2958471a85fd985013f0cd623a98a47db4fd..a5e495b678274fd5cb3a1b0581a9c9a8c89db48b 100644 (file)
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -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,8 +2173,10 @@ again:
                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 */
@@ -2211,8 +2205,6 @@ again:
        }
 
 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;

diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index c1b60299612723ebc4cef93a0574a905d76ec780..ee7299e6dea911a0568a99d802947886bdede3c4 100644 (file)
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -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;
 

diff --git a/include/linux/bio.h b/include/linux/bio.h
index ca22b06700a94d319a443f4d38225f7755e2e958..2c5806997bbf71a5edefd641fbd8e0d05be23111 100644 (file)
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -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);
 }

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 1ef99790f6ed3eee3c11498571ffd6d5c5010e4f..41afb4cfb9b0d4d7c928aa9cddd69b8ddf1bc081 100644 (file)
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -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. */