On Sat, Apr 12, 2025 at 08:37:28AM +0800, Yu Kuai wrote:
> Hi,
>
> 在 2025/04/11 23:01, Ming Lei 写道:
> > 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@xxxxxxxxxx>
> > > > Cc: Josef Bacik <josef@xxxxxxxxxxxxxx>
> > > > Cc: Yu Kuai <yukuai3@xxxxxxxxxx>
> > > > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > > > ---
> > > > 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), :-(
>
> Yes, I just didn't review in detail when I see this set is applied...
> >
> > >
> > > 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.
>
> But the problem is that in this patch, slice_start is set to *jiffies*,
> keep the old disp filed that is between old slice_start to jiffies does
> not make sense.
> >
> > 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.
> >
>
> In the original carryover calculation, bytes_disp is always set to 0,
> while slice start is set to jiffies. Patched version actually will be
> less than old version if bytes_disp is not 0;
Indeed, you are right, care to send one fix?
Otherwise, please let me know, and I can do it too.
Thanks,
Ming
