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

[Geert Uytterhoeven]

Hi David,

On Fri, May 21, 2021 at 5:18 PM David Sterba <dsterba@xxxxxxx> wrote:
> On Thu, May 20, 2021 at 03:14:03PM +0200, Arnd Bergmann wrote:
> > On Thu, May 20, 2021 at 9:43 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > On Tue, 18 May 2021, David Sterba wrote:
> > > > +     /* 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().
>
> It's in miliseconds and the division above is int/int, the problematic
> one is below.

Yep, sorry for the wrong pointer.

> >
> > > > +             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.
>
> I'll use msecs_to_jiffies, thanks. If 'hr' in schedule_hrtimeout stands
> for high resolution, it's not necessary here.

msecs_to_jiffies() takes (32-bit) "unsigned int", while delta is "s64".

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds