Re: [PATCH RESEND v6 1/8] blk-throttle: fix that io throttle can only work for single bio

[Tejun Heo <tj@kernel.org>]

Hello,

On Fri, Jul 29, 2022 at 02:32:36PM +0800, Yu Kuai wrote:
> We need to make sure following conditions is always hold:
> 
> 1) If a bio is splited, iops limits should count multiple times, while
> bps limits should only count once.
> 2) If a bio is issued while some bios are already throttled, bps limits
> should not be ignored.
> 
> commit 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
> fixes that 1) is not hold, while it breaks 2). Root cause is that such
> bio will be flaged in __blk_throtl_bio(), and later
> tg_with_in_bps_limit() will skip flaged bio.
> 
> In order to fix this problem, at first, I change that flaged bio won't
> be skipped in tg_with_in_bps_limit():
> 
> -	if (!bio_flagged(bio, BIO_THROTTLED)) {
> -		tg->bytes_disp[rw] += bio_size;
> -		tg->last_bytes_disp[rw] += bio_size;
> -	}
> -
> +	tg->bytes_disp[rw] += bio_size;
> +	tg->last_bytes_disp[rw] += bio_size;
> 
> However, this will break that bps limits should only count once. Thus I
> try to restore the overaccounting in __blk_throtl_bio() in such case:
> 
> +		if (bio_flagged(bio, BIO_THROTTLED)) {
> +			unsigned int bio_size = throtl_bio_data_size(bio);
> +
> +			if (tg->bytes_disp[rw] >= bio_size)
> +				tg->bytes_disp[rw] -= bio_size;
> +			if (tg->last_bytes_disp[rw] >= bio_size)
> +				tg->last_bytes_disp[rw] -= bio_size;
> +		}
> 
> If new slice is not started, then the decrement should make sure this
> bio won't be counted again. However, if new slice is started and the
> condition 'bytes_disp >= bio_size' doesn't hold, this bio will end up
> accounting twice.
> 
> Pleas let me know if you think this suituation is problematic, I'll try
> to figure out a new way...

While a bit tricky, I think it's fine but please add comments in the code
explaining what's going on and why. Also, can you please explain why
__blk_throtl_bio() being skipped when iops limit is not set doesn't skew the
result?

Thanks.

-- 
tejun