Re: [PATCH 2/2] blk-throttle: fix off-by-one jiffies wait_time

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

On Tue, Feb 25, 2025 at 07:09:30PM +0800, Yu Kuai wrote:
> Hi,
> 
> 在 2025/02/25 16:21, Ming Lei 写道:
> > On Tue, Feb 25, 2025 at 11:12:24AM +0800, Yu Kuai wrote:
> > > Hi, Ming!
> > > 
> > > 在 2025/02/25 10:28, Ming Lei 写道:
> > > > Can you explain in details why it signals that the rate is expected now?
> > > > 
> > > > If rate isn't expected, it will cause trouble to trim, even just the
> > > > previous part.
> > > 
> > > Ok, for example, assume bps_limit is 1000bytes, 1 jiffes is 10ms, and
> > > slice is 20ms(2 jiffies).
> > > 
> > 
> > We all know how it works, but I didn't understand the behind idea why it
> > is correct. Now I figured it out:
> > 
> > 1) increase default slice window to 2 * td->throttle_slice
> > 
> > 2) slice window is set as [jiffies - td->throttle_slice, jiffies + td->throttle_slice]
> > 
> > 3) initialize td->bytes_disp[]/td->io_dis[] as actual dispatched bytes/ios
> > done [jiffies - td->throttle_slice, 0]
> > 
> > This approach looks smart, and it should work well for any deviation which is <= 1
> > throttle_slice.
> > 
> > Probably it is enough for fixing the issue in throtl/001, even though 2 jiffies
> > timer drift still may be observed, see the below log collected in my VM(HZ_100)
> > by just running one time of blktests './check throtl':
> > 
> > @timer_expire_delay:
> > [1, 2)               387 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> > [2, 3)                11 |@                                                   |
> > 
> > bpftrace -e 'kfunc:throtl_pending_timer_fn { @timer_expire_delay = lhist(jiffies - args->t->expires, 0, 16, 1);}'
> > 
> > 
> > Also I'd suggest to remove ->carryover_bytes/ios since blk-throttle algorithm is
> > supposed to be adaptive, and the approach I suggested may cover this area,
> > what do you think of this cleanup? I have one local patchset, which can
> > pass all blktest throtl tests with removing ->carryover_bytes/ios.
> > 
> 
> It's always welcome for such cleanup. BTW, do you have plans to support
> bio merge for iops limit in blk-throttle?
> Since bio split is handled. I
> was thinking about using carryover_ios, perhaps you can handle this as
> well.

I don't know the two problems.

Let's focus on fixing throtl/001 first.

I raised the cleanup on carryover_ios because the fix I proposed in [1]
may help to cover carryover_ios too.

But I guess your patch of doubling splice window is better for fixing
throtl/001, can you send a formal patch with comment for fixing this
issue first?


[1] https://lore.kernel.org/linux-block/Z7nAJSKGANoC0Glb@fedora/



Thanks,
Ming