Re: [PATCH 2/4] btrfs: discard: save discard delay as ns not jiffy

From: Amy Parker <enbyamy@gmail.com>
To: Pavel Begunkov <asml.silence@gmail.com>
Cc: Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>,
	David Sterba <dsterba@suse.com>,
	Btrfs BTRFS <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH 2/4] btrfs: discard: save discard delay as ns not jiffy
Date: Wed, 4 Nov 2020 08:46:08 -0800	[thread overview]
Message-ID: <CAE1WUT4VGT3zA1OkiH8S3wCTw8D-5=-c0psGCWndJt+M6UgGYg@mail.gmail.com> (raw)
In-Reply-To: <bcd88bb1-37e6-2b1d-6fe8-390d3aa68d29@gmail.com>

On Wed, Nov 4, 2020 at 7:51 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 04/11/2020 15:35, Amy Parker wrote:
> > On Wed, Nov 4, 2020 at 1:52 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
> >>
> >> Most of calculations are done in ns or ms, so store discard_ctl->delay
> >> in ms and convert the final delay to jiffies only in the end.
> >
> > Great idea.
> >
> >>
> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> >> ---
> >>  fs/btrfs/ctree.h   |  2 +-
> >>  fs/btrfs/discard.c | 14 +++++++-------
> >>  2 files changed, 8 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> >> index aac3d6f4e35b..d43a82dcdfc0 100644
> >> --- a/fs/btrfs/ctree.h
> >> +++ b/fs/btrfs/ctree.h
> >> @@ -472,7 +472,7 @@ struct btrfs_discard_ctl {
> >>         atomic_t discardable_extents;
> >>         atomic64_t discardable_bytes;
> >>         u64 max_discard_size;
> >> -       unsigned long delay;
> >> +       u64 delay_ms;
> >
> > Thanks for converting this from the ambiguous unsigned long to the
> > more specific u64.
> >
> >>         u32 iops_limit;
> >>         u32 kbps_limit;
> >>         u64 discard_extent_bytes;
> >> diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c
> >> index 76796a90e88d..b6c68e5711f0 100644
> >> --- a/fs/btrfs/discard.c
> >> +++ b/fs/btrfs/discard.c
> >> @@ -355,7 +355,7 @@ void btrfs_discard_schedule_work(struct btrfs_discard_ctl *discard_ctl,
> >>
> >>         block_group = find_next_block_group(discard_ctl, now);
> >>         if (block_group) {
> >> -               unsigned long delay = discard_ctl->delay;
> >> +               u64 delay = discard_ctl->delay_ms * NSEC_PER_MSEC;
> >
> > I worry about a potential performance impact with this, but it should be
> > minimal at most.
>
> That's nothing, nsecs_to_jiffies() in the end is heavier, but this is not
> in a hot path and by all means it's heavily amortised by actual discarding.

Alright, sounds good.

>
> >
> >>                 u32 kbps_limit = READ_ONCE(discard_ctl->kbps_limit);
> >>
> >>                 /*
> >> @@ -366,9 +366,9 @@ void btrfs_discard_schedule_work(struct btrfs_discard_ctl *discard_ctl,
> >>                 if (kbps_limit && discard_ctl->prev_discard) {
> >>                         u64 bps_limit = ((u64)kbps_limit) * SZ_1K;
> >>                         u64 bps_delay = div64_u64(discard_ctl->prev_discard *
> >> -                                                 MSEC_PER_SEC, bps_limit);
> >> +                                                 NSEC_PER_SEC, bps_limit);
> >>
> >> -                       delay = max(delay, msecs_to_jiffies(bps_delay));
> >> +                       delay = max(delay, bps_delay);
> >
> > Great that we got this down to just passing max() a value. Same thing on
> > the instance below.
> >
> >>                 }
> >>
> >>                 /*
> >> @@ -378,11 +378,11 @@ void btrfs_discard_schedule_work(struct btrfs_discard_ctl *discard_ctl,
> >>                 if (now < block_group->discard_eligible_time) {
> >>                         u64 bg_timeout = block_group->discard_eligible_time - now;
> >>
> >> -                       delay = max(delay, nsecs_to_jiffies(bg_timeout));
> >> +                       delay = max(delay, bg_timeout);
> >>                 }
> >>
> >>                 mod_delayed_work(discard_ctl->discard_workers,
> >> -                                &discard_ctl->work, delay);
> >> +                                &discard_ctl->work, nsecs_to_jiffies(delay));
> >>         }
> >>  out:
> >>         spin_unlock(&discard_ctl->lock);
> >> @@ -555,7 +555,7 @@ void btrfs_discard_calc_delay(struct btrfs_discard_ctl *discard_ctl)
> >>
> >>         delay = clamp(delay, BTRFS_DISCARD_MIN_DELAY_MSEC,
> >>                       BTRFS_DISCARD_MAX_DELAY_MSEC);
> >> -       discard_ctl->delay = msecs_to_jiffies(delay);
> >> +       discard_ctl->delay_ms = delay;
> >>
> >>         spin_unlock(&discard_ctl->lock);
> >>  }
> >> @@ -687,7 +687,7 @@ void btrfs_discard_init(struct btrfs_fs_info *fs_info)
> >>         atomic_set(&discard_ctl->discardable_extents, 0);
> >>         atomic64_set(&discard_ctl->discardable_bytes, 0);
> >>         discard_ctl->max_discard_size = BTRFS_ASYNC_DISCARD_DEFAULT_MAX_SIZE;
> >> -       discard_ctl->delay = BTRFS_DISCARD_MAX_DELAY_MSEC;
> >> +       discard_ctl->delay_ms = BTRFS_DISCARD_MAX_DELAY_MSEC;
> >>         discard_ctl->iops_limit = BTRFS_DISCARD_MAX_IOPS;
> >>         discard_ctl->kbps_limit = 0;
> >>         discard_ctl->discard_extent_bytes = 0;
> >> --
> >> 2.24.0
> >>
> >
> > Looks all fine to me.
>
>
> --
> Pavel Begunkov

In this case, I see nothing more to consider with this.

Best regards,
Amy Parker
(they/them)