Re: [PATCH 2/7] blk-throttle: Refactor tg_dispatch_time by extracting tg_dispatch_bps/iops_time

[Zizhi Wo <wozizhi@xxxxxxxxxx>]

在 2025/4/15 10:19, Yu Kuai 写道:
> Hi, one nit below:
>
> 在 2025/04/14 21:27, Zizhi Wo 写道:
> > tg_dispatch_time() contained both bps and iops throttling logic. We now
> > split its internal logic into tg_dispatch_bps/iops_time() to improve code
> > consistency for future separation of the bps and iops queues.
> >
> > Besides, merge time_before() from caller into throtl_extend_slice() to 
> > make
> > code cleaner.
> >
> > Signed-off-by: Zizhi Wo <wozizhi@xxxxxxxxxx>
> > ---
> >   block/blk-throttle.c | 98 +++++++++++++++++++++++++-------------------
> >   1 file changed, 55 insertions(+), 43 deletions(-)
> >
> > diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> > index dc23c961c028..0633ae0cce90 100644
> > --- a/block/blk-throttle.c
> > +++ b/block/blk-throttle.c
> > @@ -520,6 +520,9 @@ static inline void throtl_set_slice_end(struct 
> > throtl_grp *tg, bool rw,
> >   static inline void throtl_extend_slice(struct throtl_grp *tg, bool rw,
> >                          unsigned long jiffy_end)
> >   {
> > +    if (!time_before(tg->slice_end[rw], jiffy_end))
> > +        return;
> > +
> >       throtl_set_slice_end(tg, rw, jiffy_end);
> >       throtl_log(&tg->service_queue,
> >              "[%c] extend slice start=%lu end=%lu jiffies=%lu",
> > @@ -682,10 +685,6 @@ static unsigned long tg_within_iops_limit(struct 
> > throtl_grp *tg, struct bio *bio
> >       int io_allowed;
> >       unsigned long jiffy_elapsed, jiffy_wait, jiffy_elapsed_rnd;
> > -    if (iops_limit == UINT_MAX) {
> > -        return 0;
> > -    }
> > -
> >       jiffy_elapsed = jiffies - tg->slice_start[rw];
> >       /* Round up to the next throttle slice, wait time must be 
> > nonzero */
> > @@ -711,11 +710,6 @@ static unsigned long tg_within_bps_limit(struct 
> > throtl_grp *tg, struct bio *bio,
> >       unsigned long jiffy_elapsed, jiffy_wait, jiffy_elapsed_rnd;
> >       unsigned int bio_size = throtl_bio_data_size(bio);
> > -    /* no need to throttle if this bio's bytes have been accounted */
> > -    if (bps_limit == U64_MAX || bio_flagged(bio, BIO_BPS_THROTTLED)) {
> > -        return 0;
> > -    }
> > -
> >       jiffy_elapsed = jiffy_elapsed_rnd = jiffies - tg->slice_start[rw];
> >       /* Slice has just started. Consider one slice interval */
> > @@ -742,6 +736,54 @@ static unsigned long tg_within_bps_limit(struct 
> > throtl_grp *tg, struct bio *bio,
> >       return jiffy_wait;
> >   }
> > +/*
> > + * If previous slice expired, start a new one otherwise renew/extend 
> > existing
> > + * slice to make sure it is at least throtl_slice interval long since 
> > now.
> > + * New slice is started only for empty throttle group. If there is 
> > queued bio,
> > + * that means there should be an active slice and it should be 
> > extended instead.
> > + */
> > +static void tg_update_slice(struct throtl_grp *tg, bool rw)
> > +{
> > +    if (throtl_slice_used(tg, rw) && !(tg->service_queue.nr_queued[rw]))
> > +        throtl_start_new_slice(tg, rw, true);
> > +    else
> > +        throtl_extend_slice(tg, rw, jiffies + tg->td->throtl_slice);
> > +}
> > +
> > +static unsigned long tg_dispatch_bps_time(struct throtl_grp *tg, 
> > struct bio *bio)
> > +{
> > +    bool rw = bio_data_dir(bio);
> > +    u64 bps_limit = tg_bps_limit(tg, rw);
> > +    unsigned long bps_wait;
> > +
> > +    /* no need to throttle if this bio's bytes have been accounted */
> > +    if (bps_limit == U64_MAX || tg->flags & THROTL_TG_CANCELING ||
> > +        bio_flagged(bio, BIO_BPS_THROTTLED))
> > +        return 0;
> > +
> > +    tg_update_slice(tg, rw);
> > +    bps_wait = tg_within_bps_limit(tg, bio, bps_limit);
>
> if (bps_wait > 0)

Since the time_before() logic has been incorporated into
throtl_extend_slice(), can we simplify the code by not adding the extra
check?

Thanks,
Zizhi Wo

> > +    throtl_extend_slice(tg, rw, jiffies + bps_wait);
> > +
> > +    return bps_wait;
> > +}
> > +
> > +static unsigned long tg_dispatch_iops_time(struct throtl_grp *tg, 
> > struct bio *bio)
> > +{
> > +    bool rw = bio_data_dir(bio);
> > +    u32 iops_limit = tg_iops_limit(tg, rw);
> > +    unsigned long iops_wait;
> > +
> > +    if (iops_limit == UINT_MAX || tg->flags & THROTL_TG_CANCELING)
> > +        return 0;
> > +
> > +    tg_update_slice(tg, rw);
> > +    iops_wait = tg_within_iops_limit(tg, bio, iops_limit);
>
> if (iops_wait > 0)
>
> Otherwise, LGTM
>
> Thanks,
> Kuai
> > +    throtl_extend_slice(tg, rw, jiffies + iops_wait);
> > +
> > +    return iops_wait;
> > +}
> > +
> >   /*
> >    * Returns approx number of jiffies to wait before this bio is 
> > with-in IO rate
> >    * and can be dispatched.
> > @@ -749,9 +791,7 @@ static unsigned long tg_within_bps_limit(struct 
> > throtl_grp *tg, struct bio *bio,
> >   static unsigned long tg_dispatch_time(struct throtl_grp *tg, struct 
> > bio *bio)
> >   {
> >       bool rw = bio_data_dir(bio);
> > -    unsigned long bps_wait, iops_wait, max_wait;
> > -    u64 bps_limit = tg_bps_limit(tg, rw);
> > -    u32 iops_limit = tg_iops_limit(tg, rw);
> > +    unsigned long bps_wait, iops_wait;
> >       /*
> >         * Currently whole state machine of group depends on first bio
> > @@ -762,38 +802,10 @@ static unsigned long tg_dispatch_time(struct 
> > throtl_grp *tg, struct bio *bio)
> >       BUG_ON(tg->service_queue.nr_queued[rw] &&
> >              bio != throtl_peek_queued(&tg->service_queue.queued[rw]));
> > -    /* If tg->bps = -1, then BW is unlimited */
> > -    if ((bps_limit == U64_MAX && iops_limit == UINT_MAX) ||
> > -        tg->flags & THROTL_TG_CANCELING)
> > -        return 0;
> > -
> > -    /*
> > -     * If previous slice expired, start a new one otherwise renew/extend
> > -     * existing slice to make sure it is at least throtl_slice interval
> > -     * long since now. New slice is started only for empty throttle 
> > group.
> > -     * If there is queued bio, that means there should be an active
> > -     * slice and it should be extended instead.
> > -     */
> > -    if (throtl_slice_used(tg, rw) && !(tg->service_queue.nr_queued[rw]))
> > -        throtl_start_new_slice(tg, rw, true);
> > -    else {
> > -        if (time_before(tg->slice_end[rw],
> > -            jiffies + tg->td->throtl_slice))
> > -            throtl_extend_slice(tg, rw,
> > -                jiffies + tg->td->throtl_slice);
> > -    }
> > -
> > -    bps_wait = tg_within_bps_limit(tg, bio, bps_limit);
> > -    iops_wait = tg_within_iops_limit(tg, bio, iops_limit);
> > -    if (bps_wait + iops_wait == 0)
> > -        return 0;
> > -
> > -    max_wait = max(bps_wait, iops_wait);
> > -
> > -    if (time_before(tg->slice_end[rw], jiffies + max_wait))
> > -        throtl_extend_slice(tg, rw, jiffies + max_wait);
> > +    bps_wait = tg_dispatch_bps_time(tg, bio);
> > +    iops_wait = tg_dispatch_iops_time(tg, bio);
> > -    return max_wait;
> > +    return max(bps_wait, iops_wait);
> >   }
> >   static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio)