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

[Pavel Begunkov <asml.silence@gmail.com>]

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.

> 
>>                 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