Re: [PATCH] block: throttle: don't add one extra jiffy mistakenly for bps limit

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

On Fri, Feb 21, 2025 at 02:29:30PM +0800, Yu Kuai wrote:
> Hi,
> 
> 在 2025/02/21 12:18, Ming Lei 写道:
> > > -       jiffy_wait = div64_u64(extra_bytes * HZ, bps_limit);
> > > -
> > > -       if (!jiffy_wait)
> > > -               jiffy_wait = 1;
> > > +       jiffy_wait = div64_u64_rem(extra_bytes * HZ, bps_limit,
> > > &carryover_bytes);
> > > +       tg->carryover_bytes[rw] -= div64_u64(carryover_bytes, HZ);
> > Can you explain a bit why `carryover_bytes/HZ` is subtracted instead of
> > carryover_bytes?
> 
> For example, if bps_limit is 1000, extra_bytes is 9, then:
> 
> jiffy_wait = (9 * 100) / 1000 = 0;
> carryover_bytes = (9 * 100) % 1000 = 900;
> 
> Hence we need to divide it by HZ:
> tg->carryover_bytes = 0 - 900/100 = -9;
> 
> -9 can be considered debt, and for the next IO, the bytes_allowed will
> consider the carryover_bytes.

Got it, it is just because the dividend is 'extra_bytes * HZ', so the remainder
need to be divided by HZ.

> > 
> > Also tg_within_bps_limit() may return 0 now, which isn't expected.
> 
> I think it's expected, this IO will now be dispatched directly instead
> of wait for one more jiffies, and debt will be paid if there are
> following IOs.

OK.

> 
> And if the tg idle for a long time before dispatching the next IO,
> tg_trim_slice() should handle this case and avoid long slice end. This
> is not quite handy, perhaps it's better to add a helper like
> tg_in_debt() before throtl_start_new_slice() to hande this case.
> 
> BTW, we must update the comment about carryover_bytes/ios, it's alredy
> used as debt.

Indeed, the approach is similar with the handling for
bio_issue_as_root_blkg().


Tested-by: Ming Lei <ming.lei@redhat.com>



Thanks,
Ming