Re: [PATCH v7 1/9] blk-throttle: fix that io throttle can only work for single bio

[Yu Kuai]

Hi, Tejun

在 2022/08/22 15:25, Tejun Heo 写道:
> Hello,
>
> On Mon, Aug 22, 2022 at 11:06:44AM +0800, Yu Kuai wrote:
> > While implementing the new method, I found that there seems to be a
> > misunderstanding here, the code seems to try to add flag to split bio
> > so that it won't be throttled again for bps limit, however:
> >
> > 1) for blk throttle, split bio is issued directly and will never be
> > throttled again, while orignal bio will go through throttle path again.
> > 2) if cloned bio is directed to a new disk, the flag is cleared anyway.
>
> Doesn't that make the current code correct then? But you were seeing
> incorrect behaviors, right?

According to the commit message in commit 111be8839817 ("block-throttle:
avoid double charge"):

If the bio is cloned/split, we copy the flag to new bio too to avoid a
double charge.

Which make me think the split bio will be resubmitted, and after
implementing the new solution, I found that test result is not as
expected. After spending sometime figuring out what is wrong, I found
that split bio will be dispatched directly from caller, while orignal
bio will be resubmitted.

I guess commit 111be8839817 made a mistake, however, there should be
no problem because orignal bio is flagged already, and it's handled
correctly.

Anyway, I removed the code in __bio_clone() and check flag in
__bio_split_to_limits in my patch:
--- 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-merge.c b/block/blk-merge.c
index ff04e9290715..10330bb038ca 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -358,6 +358,13 @@ struct bio *__bio_split_to_limits(struct bio *bio, 
struct queue_limits *lim,
                blkcg_bio_issue_init(split);
                bio_chain(split, bio);
                trace_block_split(split, bio->bi_iter.bi_sector);
+
+               /*
+                * original bio will be resubmited and throttled again, 
clear
+                * the iops flag so that it can be count again for iops 
limit.
+                */
+               if (bio_flagged(bio, BIO_IOPS_THROTTLED))
+                       bio_clear_flag(bio, BIO_IOPS_THROTTLED);
                submit_bio_noacct(bio);
                return split;
        }


> Thanks.