Re: [PATCH 3/3] blk-throttle: carry over directly

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

On Fri, Apr 11, 2025 at 10:53:12AM +0800, Yu Kuai wrote:
> Hi, Ming
> 
> 在 2025/03/05 12:31, Ming Lei 写道:
> > Now ->carryover_bytes[] and ->carryover_ios[] only covers limit/config
> > update.
> > 
> > Actually the carryover bytes/ios can be carried to ->bytes_disp[] and
> > ->io_disp[] directly, since the carryover is one-shot thing and only valid
> > in current slice.
> > 
> > Then we can remove the two fields and simplify code much.
> > 
> > Type of ->bytes_disp[] and ->io_disp[] has to change as signed because the
> > two fields may become negative when updating limits or config, but both are
> > big enough for holding bytes/ios dispatched in single slice
> > 
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Josef Bacik <josef@toxicpanda.com>
> > Cc: Yu Kuai <yukuai3@huawei.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >   block/blk-throttle.c | 49 +++++++++++++++++++-------------------------
> >   block/blk-throttle.h |  4 ++--
> >   2 files changed, 23 insertions(+), 30 deletions(-)
> > 
> > diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> > index 7271aee94faf..91dab43c65ab 100644
> > --- a/block/blk-throttle.c
> > +++ b/block/blk-throttle.c
> > @@ -478,8 +478,6 @@ static inline void throtl_start_new_slice_with_credit(struct throtl_grp *tg,
> >   {
> >   	tg->bytes_disp[rw] = 0;
> >   	tg->io_disp[rw] = 0;
> > -	tg->carryover_bytes[rw] = 0;
> > -	tg->carryover_ios[rw] = 0;
> >   	/*
> >   	 * Previous slice has expired. We must have trimmed it after last
> > @@ -498,16 +496,14 @@ static inline void throtl_start_new_slice_with_credit(struct throtl_grp *tg,
> >   }
> >   static inline void throtl_start_new_slice(struct throtl_grp *tg, bool rw,
> > -					  bool clear_carryover)
> > +					  bool clear)
> >   {
> > -	tg->bytes_disp[rw] = 0;
> > -	tg->io_disp[rw] = 0;
> > +	if (clear) {
> > +		tg->bytes_disp[rw] = 0;
> > +		tg->io_disp[rw] = 0;
> > +	}
> >   	tg->slice_start[rw] = jiffies;
> >   	tg->slice_end[rw] = jiffies + tg->td->throtl_slice;
> > -	if (clear_carryover) {
> > -		tg->carryover_bytes[rw] = 0;
> > -		tg->carryover_ios[rw] = 0;
> > -	}
> >   	throtl_log(&tg->service_queue,
> >   		   "[%c] new slice start=%lu end=%lu jiffies=%lu",
> > @@ -617,20 +613,16 @@ static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw)
> >   	 */
> >   	time_elapsed -= tg->td->throtl_slice;
> >   	bytes_trim = calculate_bytes_allowed(tg_bps_limit(tg, rw),
> > -					     time_elapsed) +
> > -		     tg->carryover_bytes[rw];
> > -	io_trim = calculate_io_allowed(tg_iops_limit(tg, rw), time_elapsed) +
> > -		  tg->carryover_ios[rw];
> > +					     time_elapsed);
> > +	io_trim = calculate_io_allowed(tg_iops_limit(tg, rw), time_elapsed);
> >   	if (bytes_trim <= 0 && io_trim <= 0)
> >   		return;
> > -	tg->carryover_bytes[rw] = 0;
> >   	if ((long long)tg->bytes_disp[rw] >= bytes_trim)
> >   		tg->bytes_disp[rw] -= bytes_trim;
> >   	else
> >   		tg->bytes_disp[rw] = 0;
> > -	tg->carryover_ios[rw] = 0;
> >   	if ((int)tg->io_disp[rw] >= io_trim)
> >   		tg->io_disp[rw] -= io_trim;
> >   	else
> > @@ -645,7 +637,8 @@ static inline void throtl_trim_slice(struct throtl_grp *tg, bool rw)
> >   		   jiffies);
> >   }
> > -static void __tg_update_carryover(struct throtl_grp *tg, bool rw)
> > +static void __tg_update_carryover(struct throtl_grp *tg, bool rw,
> > +				  long long *bytes, int *ios)
> >   {
> >   	unsigned long jiffy_elapsed = jiffies - tg->slice_start[rw];
> >   	u64 bps_limit = tg_bps_limit(tg, rw);
> > @@ -658,26 +651,28 @@ static void __tg_update_carryover(struct throtl_grp *tg, bool rw)
> >   	 * configuration.
> >   	 */
> >   	if (bps_limit != U64_MAX)
> > -		tg->carryover_bytes[rw] +=
> > -			calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
> > +		*bytes = calculate_bytes_allowed(bps_limit, jiffy_elapsed) -
> >   			tg->bytes_disp[rw];
> >   	if (iops_limit != UINT_MAX)
> > -		tg->carryover_ios[rw] +=
> > -			calculate_io_allowed(iops_limit, jiffy_elapsed) -
> > +		*ios = calculate_io_allowed(iops_limit, jiffy_elapsed) -
> >   			tg->io_disp[rw];
> > +	tg->bytes_disp[rw] -= *bytes;
> > +	tg->io_disp[rw] -= *ios;
> 
> This patch is applied before I get a chance to review. :( I think the
> above update should be:

oops, your review period takes too long(> 1 month), :-(

> 
> tg->bytes_disp[rw] = -*bytes;
> tg->io_disp[rw] = -*ios;

I think the above is wrong since it simply override the existed dispatched
bytes/ios.

The calculation can be understood from two ways:

1) delta = calculate_bytes_allowed(bps_limit, jiffy_elapsed) - tg->bytes_disp[rw];

`delta` represents difference between theoretical and actual dispatch bytes.

If `delta` > 0, it means we dispatch too less in past, and we have to subtract
`delta` from ->bytes_disp, so that in future we can dispatch more.

Similar with 'delta < 0'.

2) from consumer viewpoint:

tg_within_bps_limit(): patched

	...
	bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed_rnd);
	if (bytes_allowed > 0 && tg->bytes_disp[rw] + bio_size <= bytes_allowed)
		...

tg_within_bps_limit(): before patched
	...
    bytes_allowed = calculate_bytes_allowed(bps_limit, jiffy_elapsed_rnd) +
		tg->carryover_bytes[rw];
	if (bytes_allowed > 0 && tg->bytes_disp[rw] + bio_size <= bytes_allowed)
		...

So if `delta` is subtracted from `bytes_allowed` in patched code, we should
subtract same bytes from ->byte_disp[] side for maintaining the relation.


> 
> Otherwise, the result is actually (2 * disp - allowed), which might be a
> huge value, causing long dealy for the next dispatch.
> 
> This is what the old carryover fileds do, above change will work, but
> look wried.

As I explained, the patched code follows the original carryover calculation, and it
passes all throt blktests.

Or do you have test case broken by this patch?



Thanks.
Ming