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:50 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 04/11/2020 17:33, Amy Parker wrote:
> > 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.
>
> Ahh, ok. It's handled and I left it as it was, that BTW is still a problem.

How often is iops_limit unfindable?

>
> First it calculates a delay based on number of queued extents and than clamps
> it to (BTRFS_DISCARD_MIN_DELAY_MSEC, BTRFS_DISCARD_MAX_DELAY_MSEC). Without
> this patch it did the same but the lower bound was calculated from iops_limit.

Thanks for clarifying.

>
> > Async discard was added in 5.6, correct? So yeah, makes sense then that people
> > just had their hands full. Thanks for adding it.
>
> b0643e59cfa609c4b5f ("btrfs: add the beginning of async discard, discard
> workqueue"). Dec 2019, so less than a year

Thanks for finding the commit.

>
> >
> >>
> >>>
> >>>>
> >>>> -       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.
>
> --
> Pavel Begunkov

Best regards,
Amy Parker
(they/them)