Re: [PATCH 1/4] btrfs: discard: speed up discard up to iops_limit

[Amy Parker <enbyamy@gmail.com>]

On Wed, Nov 4, 2020 at 9:22 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 04/11/2020 15:29, Amy Parker wrote:
> > On Wed, Nov 4, 2020 at 1:50 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
> >>
> >> Instead of using iops_limit only for cutting off extremes, calculate the
> >> discard delay directly from it, so it closely follows iops_limit and
> >> doesn't under-discarding even though quotas are not saturated.
> >
> > This sounds like it potentially be a great performance boost, do you
> > have any performance metrics regarding this patch?
>
> Boosting the discard rate and so reaping stalling blocks may be nice, but
> unless it holds too much memory creating lack of space it shouldn't affect
> throughput. Though, it's better to ask people with deeper understanding
> of the fs.

Alright, thanks for the clarification.

> What I've seen is that in some cases there are extents staying queued for
> discarding for _too_ long. E.g. reaping a small number of very fat extents
> keeps delay at max and doesn't allow to reap them effectively. That could
> be a problem with fast drives.

Ah, yep. Seen this personally to a smaller extent.

>
> >
> >>
> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> >> ---
> >>  fs/btrfs/discard.c | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c
> >> index 741c7e19c32f..76796a90e88d 100644
> >> --- a/fs/btrfs/discard.c
> >> +++ b/fs/btrfs/discard.c
> >> @@ -519,7 +519,6 @@ void btrfs_discard_calc_delay(struct btrfs_discard_ctl *discard_ctl)
> >>         s64 discardable_bytes;
> >>         u32 iops_limit;
> >>         unsigned long delay;
> >> -       unsigned long lower_limit = BTRFS_DISCARD_MIN_DELAY_MSEC;
> >>
> >>         discardable_extents = atomic_read(&discard_ctl->discardable_extents);
> >>         if (!discardable_extents)
> >> @@ -550,11 +549,12 @@ void btrfs_discard_calc_delay(struct btrfs_discard_ctl *discard_ctl)
> >>
> >>         iops_limit = READ_ONCE(discard_ctl->iops_limit);
> >>         if (iops_limit)
> >> -               lower_limit = max_t(unsigned long, lower_limit,
> >> -                                   MSEC_PER_SEC / iops_limit);
> >> +               delay = MSEC_PER_SEC / iops_limit;
> >> +       else
> >> +               delay = BTRFS_DISCARD_TARGET_MSEC / discardable_extents;
> >
> > Looks good to me. I wonder why there wasn't handling of if iops_limit
> > was unfindable
> > before?
>
> Not sure what you mean by unfindable, but async discard is relatively new,
> might be that everyone just have their hands full.

By unfindable I mean if iops_limit turned up as null when reading it
from discard_ctl.
Async discard was added in 5.6, correct? So yeah, makes sense then that people
just had their hands full. Thanks for adding it.

>
> >
> >>
> >> -       delay = BTRFS_DISCARD_TARGET_MSEC / discardable_extents;
> >> -       delay = clamp(delay, lower_limit, BTRFS_DISCARD_MAX_DELAY_MSEC);
> >> +       delay = clamp(delay, BTRFS_DISCARD_MIN_DELAY_MSEC,
> >> +                     BTRFS_DISCARD_MAX_DELAY_MSEC);
> >>         discard_ctl->delay = msecs_to_jiffies(delay);
> >>
> >>         spin_unlock(&discard_ctl->lock);
> >> --
> >> 2.24.0
> >>
> >
> > This patch looks all great to me.
> >
> > Best regards,
> > Amy Parker
> > (they/them)
> >
>
> --
> Pavel Begunkov

Best regards,
Amy Parker
(they/them)