On Thu, Dec 30, 2021 at 11:45:13AM +0800, Ming Lei wrote:
> 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 charge
> the bio for iops limit in blk_throtl_bio() in case that BIO_THROTTLED
> is set. 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.
>
> Reported-by: 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 | 2 +-
> block/blk-throttle.h | 8 +++++---
> 3 files changed, 6 insertions(+), 6 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 7c462c006b26..ea532c178385 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -2043,7 +2043,7 @@ static inline void throtl_update_latency_buckets(struct throtl_data *td)
> }
> #endif
>
> -void blk_throtl_charge_bio_split(struct bio *bio)
> +void blk_throtl_charge_for_iops_limit(struct bio *bio)
> {
> struct blkcg_gq *blkg = bio->bi_blkg;
> struct throtl_grp *parent = blkg_to_tg(blkg);
> diff --git a/block/blk-throttle.h b/block/blk-throttle.h
> index 175f03abd9e4..954b9cac19b7 100644
> --- a/block/blk-throttle.h
> +++ b/block/blk-throttle.h
> @@ -158,20 +158,22 @@ static inline struct throtl_grp *blkg_to_tg(struct blkcg_gq *blkg)
> static inline int blk_throtl_init(struct request_queue *q) { return 0; }
> static inline void blk_throtl_exit(struct request_queue *q) { }
> static inline void blk_throtl_register_queue(struct request_queue *q) { }
> -static inline void blk_throtl_charge_bio_split(struct bio *bio) { }
> +static inline void blk_throtl_charge_for_iops_limit(struct bio *bio) { }
> static inline bool blk_throtl_bio(struct bio *bio) { return false; }
> #else /* CONFIG_BLK_DEV_THROTTLING */
> int blk_throtl_init(struct request_queue *q);
> void blk_throtl_exit(struct request_queue *q);
> void blk_throtl_register_queue(struct request_queue *q);
> -void blk_throtl_charge_bio_split(struct bio *bio);
> +void blk_throtl_charge_for_iops_limit(struct bio *bio);
> bool __blk_throtl_bio(struct bio *bio);
> 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))
> + if (bio_flagged(bio, BIO_THROTTLED)) {
> + blk_throtl_charge_for_iops_limit(bio);
This way may cause double charge since the bio's iops limit
charge can be done via blk_throtl_dispatch_work_fn() too.
Will post another version for fix this issue.
Thanks,
Ming
