> 2022年2月15日 下午4:17，Ning Li <lining2020x@xxxxxxx> 写道：
>
> If where will be one more version of this patchset, please update my sign with
>
> `Ning Li <lining@2020x@xxxxxxx>`. : )
Sorry for a typo, it’s `Ning Li <lining2020x@xxxxxxx>`
> Thanks
>
>
>> 2022年2月9日 下午5:14，Ming Lei <ming.lei@xxxxxxxxxx> 写道：
>>
>> Commit 111be8839817 ("block-throttle: avoid double charge") marks bio as
>> BIO_THROTTLED unconditionally if __blk_throtl_bio() is called on this bio,
>> then this bio won't be called into __blk_throtl_bio() any more. This way
>> is to avoid double charge in case of bio splitting. It is reasonable for
>> read/write throughput limit, but not reasonable for IOPS limit because
>> block layer provides io accounting against split bio.
>>
>> Chunguang Xu has already observed this issue and fixed it in commit
>> 4f1e9630afe6 ("blk-throtl: optimize IOPS throttle for large IO scenarios").
>> However, that patch only covers bio splitting in __blk_queue_split(), and
>> we have other kind of bio splitting, such as bio_split() &
>> submit_bio_noacct() and other ways.
>>
>> This patch tries to fix the issue in one generic way by always charging
>> the bio for iops limit in blk_throtl_bio(). This way is reasonable:
>> re-submission & fast-cloned bio is charged if it is submitted to same
>> disk/queue, and BIO_THROTTLED will be cleared if bio->bi_bdev is changed.
>>
>> This new approach can get much more smooth/stable iops limit compared with
>> commit 4f1e9630afe6 ("blk-throtl: optimize IOPS throttle for large IO
>> scenarios") since that commit can't throttle current split bios actually.
>>
>> Also this way won't cause new double bio iops charge in
>> blk_throtl_dispatch_work_fn() in which blk_throtl_bio() won't be called
>> any more.
>>
>> Reported-by: Li Ning <lining2020x@xxxxxxx>
>> Cc: Tejun Heo <tj@xxxxxxxxxx>
>> Cc: Chunguang Xu <brookxu@xxxxxxxxxxx>
>> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
>> ---
>> block/blk-merge.c | 2 --
>> block/blk-throttle.c | 10 +++++++---
>> block/blk-throttle.h | 2 --
>> 3 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>> index 4de34a332c9f..f5255991b773 100644
>> --- a/block/blk-merge.c
>> +++ b/block/blk-merge.c
>> @@ -368,8 +368,6 @@ void __blk_queue_split(struct request_queue *q, struct bio **bio,
>> trace_block_split(split, (*bio)->bi_iter.bi_sector);
>> submit_bio_noacct(*bio);
>> *bio = split;
>> -
>> - blk_throtl_charge_bio_split(*bio);
>> }
>> }
>>
>> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
>> index 6cca1715c31e..6f2a8e7801fe 100644
>> --- a/block/blk-throttle.c
>> +++ b/block/blk-throttle.c
>> @@ -808,7 +808,8 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
>> unsigned long jiffy_elapsed, jiffy_wait, jiffy_elapsed_rnd;
>> unsigned int bio_size = throtl_bio_data_size(bio);
>>
>> - if (bps_limit == U64_MAX) {
>> + /* no need to throttle if this bio's bytes have been accounted */
>> + if (bps_limit == U64_MAX || bio_flagged(bio, BIO_THROTTLED)) {
>> if (wait)
>> *wait = 0;
>> return true;
>> @@ -920,9 +921,12 @@ 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 */
>> - tg->bytes_disp[rw] += bio_size;
>> + if (!bio_flagged(bio, BIO_THROTTLED)) {
>> + tg->bytes_disp[rw] += bio_size;
>> + tg->last_bytes_disp[rw] += bio_size;
>> + }
>> +
>> tg->io_disp[rw]++;
>> - tg->last_bytes_disp[rw] += bio_size;
>> tg->last_io_disp[rw]++;
>>
>> /*
>> diff --git a/block/blk-throttle.h b/block/blk-throttle.h
>> index 175f03abd9e4..cb43f4417d6e 100644
>> --- a/block/blk-throttle.h
>> +++ b/block/blk-throttle.h
>> @@ -170,8 +170,6 @@ static inline bool blk_throtl_bio(struct bio *bio)
>> {
>> struct throtl_grp *tg = blkg_to_tg(bio->bi_blkg);
>>
>> - if (bio_flagged(bio, BIO_THROTTLED))
>> - return false;
>> if (!tg->has_rules[bio_data_dir(bio)])
>> return false;
>>
>> --
>> 2.31.1
>
