Re: [PATCH] block: throttle: charge io re-submission for iops limit

[Ming Lei <ming.lei@redhat.com>]

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@163.com
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Chunguang Xu <brookxu@tencent.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  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