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