| Message ID | 2a3d84dfc9384eed8659963d1dafedabb3f17c75.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: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? > > 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? > > - 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)
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. 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. > >> >> 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. > >> >> - 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) >
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)
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. 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. > 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 > >> >>> >>>> >>>> - 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.
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)
On 04/11/2020 17:55, Amy Parker wrote: > 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? I don't know, but the default is 10, so shouldn't be too ubiquitous. Maybe someone here knows statistics. > >> >> 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) >
On Wed, Nov 4, 2020 at 10:09 AM Pavel Begunkov <asml.silence@gmail.com> wrote: > > On 04/11/2020 17:55, Amy Parker wrote: > > 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? > > I don't know, but the default is 10, so shouldn't be too ubiquitous. > Maybe someone here knows statistics. So it isn't a major issue right now, and we can just have it looked at whenever someone has the time to. > > > > >> > >> 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) > > > > -- > Pavel Begunkov Best regards, Amy Parker (they/them)
On 11/4/20 4:45 AM, Pavel Begunkov 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. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
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; - 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);
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. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/btrfs/discard.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)