| Message ID | 5be2ccce4a6ebe7c96274f63091a04aeba9af9d8.1604444952.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fixes for btrfs async discards | expand |
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. > 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. Best regards, Amy Parker (they/them)
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.
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)
On 11/4/20 4:45 AM, Pavel Begunkov 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. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
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; 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; 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); } /* @@ -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;
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. 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(-)