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

[David Sterba <dsterba@suse.cz>]

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@linux-m68k.org> 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.
> 
> > > +             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.