Re: [PATCH] btrfs: scrub: per-device bandwidth control

[Arnd Bergmann <arnd@arndb.de>]

On Thu, May 20, 2021 at 9:43 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, 18 May 2021, David Sterba wrote:

> > --- a/fs/btrfs/scrub.c
> > +++ b/fs/btrfs/scrub.c
> > @@ -1988,6 +1993,60 @@ static void scrub_page_put(struct scrub_page *spage)
> >       }
> > }
> >
> > +/*
> > + * Throttling of IO submission, bandwidth-limit based, the timeslice is 1
> > + * second.  Limit can be set via /sys/fs/UUID/devinfo/devid/scrub_speed_max.
> > + */
> > +static void scrub_throttle(struct scrub_ctx *sctx)
> > +{
> > +     const int time_slice = 1000;
> > +     struct scrub_bio *sbio;
> > +     struct btrfs_device *device;
> > +     s64 delta;
> > +     ktime_t now;
> > +     u32 div;
> > +     u64 bwlimit;
> > +
> > +     sbio = sctx->bios[sctx->curr];
> > +     device = sbio->dev;
> > +     bwlimit = READ_ONCE(device->scrub_speed_max);
> > +     if (bwlimit == 0)
> > +             return;
> > +
> > +     /*
> > +      * Slice is divided into intervals when the IO is submitted, adjust by
> > +      * bwlimit and maximum of 64 intervals.
> > +      */
> > +     div = max_t(u32, 1, (u32)(bwlimit / (16 * 1024 * 1024)));
> > +     div = min_t(u32, 64, div);
> > +
> > +     /* Start new epoch, set deadline */
> > +     now = ktime_get();
> > +     if (sctx->throttle_deadline == 0) {
> > +             sctx->throttle_deadline = ktime_add_ms(now, time_slice / div);
>
> ERROR: modpost: "__udivdi3" [fs/btrfs/btrfs.ko] undefined!
>
> div_u64(bwlimit, div)

If 'time_slice' is in nanoseconds, the best interface to use
is ktime_divns().

> > +             sctx->throttle_sent = 0;
> > +     }
> > +
> > +     /* Still in the time to send? */
> > +     if (ktime_before(now, sctx->throttle_deadline)) {
> > +             /* If current bio is within the limit, send it */
> > +             sctx->throttle_sent += sbio->bio->bi_iter.bi_size;
> > +             if (sctx->throttle_sent <= bwlimit / div)
> > +                     return;

Doesn't this also need to be changed?

> > +             /* We're over the limit, sleep until the rest of the slice */
> > +             delta = ktime_ms_delta(sctx->throttle_deadline, now);
> > +     } else {
> > +             /* New request after deadline, start new epoch */
> > +             delta = 0;
> > +     }
> > +
> > +     if (delta)
> > +             schedule_timeout_interruptible(delta * HZ / 1000);
>
> ERROR: modpost: "__divdi3" [fs/btrfs/btrfs.ko] undefined!
>
> I'm a bit surprised gcc doesn't emit code for the division by the
> constant 1000, but emits a call to __divdi3().  So this has to become
> div_u64(), too.

There is schedule_hrtimeout(), which takes a ktime_t directly
but has slightly different behavior. There is also an msecs_to_jiffies
helper that should produce a fast division.

       Arnd