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 10:55:23AM +0800, Ming Lei wrote:
> Hi Yukuai,
> 
> On Thu, Feb 20, 2025 at 09:38:12PM +0800, Yu Kuai wrote:
> > Hi,
> > 
> > 在 2025/02/20 19:17, Ming Lei 写道:
> > > When the current bio needs to be throttled because of bps limit, the wait
> > > time for the extra bytes may be less than 1 jiffy, tg_within_bps_limit()
> > > adds one extra 1 jiffy.
> > > 
> > > However, when taking roundup time into account, the extra 1 jiffy
> > > may become not necessary, then bps limit becomes not accurate. This way
> > > causes blktests throtl/001 failure in case of CONFIG_HZ_100=y.
> > > 
> > > Fix it by not adding the 1 jiffy in case that the roundup time can
> > > cover it.
> > > 
> > > Cc: Tejun Heo <tj@kernel.org>
> > > Cc: Yu Kuai <yukuai3@huawei.com>
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > ---
> > >   block/blk-throttle.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> > > index 8d149aff9fd0..8348972c517b 100644
> > > --- a/block/blk-throttle.c
> > > +++ b/block/blk-throttle.c
> > > @@ -729,14 +729,14 @@ static unsigned long tg_within_bps_limit(struct throtl_grp *tg, struct bio *bio,
> > >   	extra_bytes = tg->bytes_disp[rw] + bio_size - bytes_allowed;
> > >   	jiffy_wait = div64_u64(extra_bytes * HZ, bps_limit);
> > > -	if (!jiffy_wait)
> > > -		jiffy_wait = 1;
> > > -
> > >   	/*
> > >   	 * This wait time is without taking into consideration the rounding
> > >   	 * up we did. Add that time also.
> > >   	 */
> > >   	jiffy_wait = jiffy_wait + (jiffy_elapsed_rnd - jiffy_elapsed);
> > > +	if (!jiffy_wait)
> > > +		jiffy_wait = 1;
> > 
> > Just wonder, will wait (0, 1) less jiffies is better than wait (0, 1)
> > more jiffies.
> > 
> > How about following changes?
> > 
> > Thanks,
> > Kuai
> > 
> > diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> > index 8d149aff9fd0..f8430baf3544 100644
> > --- a/block/blk-throttle.c
> > +++ b/block/blk-throttle.c
> > @@ -703,6 +703,7 @@ static unsigned long tg_within_bps_limit(struct
> > throtl_grp *tg, struct bio *bio,
> >                                 u64 bps_limit)
> >  {
> >         bool rw = bio_data_dir(bio);
> > +       long long carryover_bytes;
> >         long long bytes_allowed;
> >         u64 extra_bytes;
> >         unsigned long jiffy_elapsed, jiffy_wait, jiffy_elapsed_rnd;
> > @@ -727,10 +728,11 @@ static unsigned long tg_within_bps_limit(struct
> > throtl_grp *tg, struct bio *bio,
> > 
> >         /* Calc approx time to dispatch */
> >         extra_bytes = tg->bytes_disp[rw] + bio_size - bytes_allowed;
> > -       jiffy_wait = div64_u64(extra_bytes * HZ, bps_limit);
> > +       jiffy_wait = div64_u64_rem(extra_bytes * HZ, bps_limit,
> > carryover_bytes);
> > 
> 
> &carryover_bytes
> 
> > +       /* carryover_bytes is dispatched without waiting */
> >         if (!jiffy_wait)
> > -               jiffy_wait = 1;
> > +               tg->carryover_bytes[rw] -= carryover_bytes;

Not sure ->carryover_bytes[] can be used here, the comment said
clearly it is only for updating config.

Neither it is good to add one extra, nor add one less, maybe
DIV64_U64_ROUND_CLOSEST() is better?

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 8d149aff9fd0..5791612b3543 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -727,16 +727,16 @@ static unsigned long tg_within_bps_limit(struct throtl_grp *tg, struct bio *bio,
 
 	/* Calc approx time to dispatch */
 	extra_bytes = tg->bytes_disp[rw] + bio_size - bytes_allowed;
-	jiffy_wait = div64_u64(extra_bytes * HZ, bps_limit);
-
-	if (!jiffy_wait)
-		jiffy_wait = 1;
+	jiffy_wait = DIV64_U64_ROUND_CLOSEST(extra_bytes * HZ, bps_limit);
 
 	/*
 	 * This wait time is without taking into consideration the rounding
 	 * up we did. Add that time also.
 	 */
 	jiffy_wait = jiffy_wait + (jiffy_elapsed_rnd - jiffy_elapsed);
+	if (!jiffy_wait)
+		jiffy_wait = 1;
+
 	return jiffy_wait;
 }
 


Thanks,
Ming