[PATCH v2 0/2] btrfs: adjust async discard tuning

[PATCH v2 0/2] btrfs: adjust async discard tuning

[Boris Burkov]

Since discard=async became the implicit default in btrfs in the
6.2 kernel, there have been numerous complaints about discard being
too spread out on workstation systems. This results in situations like
a users drive not being idle for an unexpectedly long period of time.

This is caused by a relatively low default iops limit of 10, so this
series raises the default limit to 1000 (1ms delay) and modifies a
weird fallback behavior for limit=0 to be interpreted as unlimited.

Link: https://lore.kernel.org/linux-btrfs/ZCxKc5ZzP3Np71IC@infradead.org/T/#m6ebdeb475809ed7714b21b8143103fb7e5a966da
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2182228
Link: https://www.reddit.com/r/archlinux/comments/121htxn/btrfs_discard_storm_on_62x_kernel/

---
Changelog:
v2: actually set the limit to 1k, not 10k.

Boris Burkov (2):
  btrfs: set default discard iops_limit to 1000
  btrfs: reinterpret async discard iops_limit=0 as no delay

 fs/btrfs/discard.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

-- 
2.40.0

[PATCH v2 1/2] btrfs: set default discard iops_limit to 1000

[Boris Burkov]

Previously, the default was a relatively conservative 10. This results
in a 100ms delay, so with ~300 discards in a commit, it takes the full
30s till the next commit to finish the discards. On a workstation, this
results in the disk never going idle, wasting power/battery, etc.

Set the default to 1000, which results in using the smallest possible
delay, currently, which is 1ms. This has shown to not pathologically
keep the disk busy by the original reporter.

Link: https://lore.kernel.org/linux-btrfs/ZCxKc5ZzP3Np71IC@infradead.org/T/#m6ebdeb475809ed7714b21b8143103fb7e5a966da
Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/discard.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c
index 317aeff6c1da..0bc526f5fcd9 100644
--- a/fs/btrfs/discard.c
+++ b/fs/btrfs/discard.c
@@ -60,7 +60,7 @@
 #define BTRFS_DISCARD_TARGET_MSEC	(6 * 60 * 60UL * MSEC_PER_SEC)
 #define BTRFS_DISCARD_MIN_DELAY_MSEC	(1UL)
 #define BTRFS_DISCARD_MAX_DELAY_MSEC	(1000UL)
-#define BTRFS_DISCARD_MAX_IOPS		(10U)
+#define BTRFS_DISCARD_MAX_IOPS		(1000U)
 
 /* Monotonically decreasing minimum length filters after index 0 */
 static int discard_minlen[BTRFS_NR_DISCARD_LISTS] = {
-- 
2.40.0

[PATCH v2 2/2] btrfs: reinterpret async discard iops_limit=0 as no delay

[Boris Burkov]

Currently, a limit of 0 results in a hardcoded metering over 6 hours.
Since the default is a set limit, I suspect no one truly depends on this
rather arbitrary setting. Repurpose it for an arguably more useful
"unlimited" mode, where the delay is 0.

Note that if block groups are too new, or go fully empty, there is still
a delay associated with those conditions. Those delays implement
heuristics for not trimming a region we are relatively likely to fully
overwrite soon.

Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/discard.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c
index 0bc526f5fcd9..c48abc817ed2 100644
--- a/fs/btrfs/discard.c
+++ b/fs/btrfs/discard.c
@@ -56,8 +56,6 @@
 #define BTRFS_DISCARD_DELAY		(120ULL * NSEC_PER_SEC)
 #define BTRFS_DISCARD_UNUSED_DELAY	(10ULL * NSEC_PER_SEC)
 
-/* Target completion latency of discarding all discardable extents */
-#define BTRFS_DISCARD_TARGET_MSEC	(6 * 60 * 60UL * MSEC_PER_SEC)
 #define BTRFS_DISCARD_MIN_DELAY_MSEC	(1UL)
 #define BTRFS_DISCARD_MAX_DELAY_MSEC	(1000UL)
 #define BTRFS_DISCARD_MAX_IOPS		(1000U)
@@ -577,6 +575,7 @@ void btrfs_discard_calc_delay(struct btrfs_discard_ctl *discard_ctl)
 	s32 discardable_extents;
 	s64 discardable_bytes;
 	u32 iops_limit;
+	unsigned long min_delay = BTRFS_DISCARD_MIN_DELAY_MSEC;
 	unsigned long delay;
 
 	discardable_extents = atomic_read(&discard_ctl->discardable_extents);
@@ -607,13 +606,16 @@ void btrfs_discard_calc_delay(struct btrfs_discard_ctl *discard_ctl)
 	}
 
 	iops_limit = READ_ONCE(discard_ctl->iops_limit);
-	if (iops_limit)
+
+	if (iops_limit) {
 		delay = MSEC_PER_SEC / iops_limit;
-	else
-		delay = BTRFS_DISCARD_TARGET_MSEC / discardable_extents;
+	} else {
+		/* unset iops_limit means go as fast as possible, so allow a delay of 0 */
+		delay = 0;
+		min_delay = 0;
+	}
 
-	delay = clamp(delay, BTRFS_DISCARD_MIN_DELAY_MSEC,
-		      BTRFS_DISCARD_MAX_DELAY_MSEC);
+	delay = clamp(delay, min_delay, BTRFS_DISCARD_MAX_DELAY_MSEC);
 	discard_ctl->delay_ms = delay;
 
 	spin_unlock(&discard_ctl->lock);
-- 
2.40.0

Re: [PATCH v2 0/2] btrfs: adjust async discard tuning

[David Sterba]

On Wed, Apr 05, 2023 at 12:43:57PM -0700, Boris Burkov wrote:
> Since discard=async became the implicit default in btrfs in the
> 6.2 kernel, there have been numerous complaints about discard being
> too spread out on workstation systems. This results in situations like
> a users drive not being idle for an unexpectedly long period of time.
> 
> This is caused by a relatively low default iops limit of 10, so this
> series raises the default limit to 1000 (1ms delay) and modifies a
> weird fallback behavior for limit=0 to be interpreted as unlimited.
> 
> Link: https://lore.kernel.org/linux-btrfs/ZCxKc5ZzP3Np71IC@infradead.org/T/#m6ebdeb475809ed7714b21b8143103fb7e5a966da
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2182228
> Link: https://www.reddit.com/r/archlinux/comments/121htxn/btrfs_discard_storm_on_62x_kernel/
> 
> ---
> Changelog:
> v2: actually set the limit to 1k, not 10k.
> 
> Boris Burkov (2):
>   btrfs: set default discard iops_limit to 1000
>   btrfs: reinterpret async discard iops_limit=0 as no delay

Added to misc-next, thanks.

Re: [PATCH v2 1/2] btrfs: set default discard iops_limit to 1000

[Neal Gompa]

On Wed, Apr 5, 2023 at 3:49 PM Boris Burkov <boris@bur.io> wrote:
>
> Previously, the default was a relatively conservative 10. This results
> in a 100ms delay, so with ~300 discards in a commit, it takes the full
> 30s till the next commit to finish the discards. On a workstation, this
> results in the disk never going idle, wasting power/battery, etc.
>
> Set the default to 1000, which results in using the smallest possible
> delay, currently, which is 1ms. This has shown to not pathologically
> keep the disk busy by the original reporter.
>
> Link: https://lore.kernel.org/linux-btrfs/ZCxKc5ZzP3Np71IC@infradead.org/T/#m6ebdeb475809ed7714b21b8143103fb7e5a966da
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>  fs/btrfs/discard.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c
> index 317aeff6c1da..0bc526f5fcd9 100644
> --- a/fs/btrfs/discard.c
> +++ b/fs/btrfs/discard.c
> @@ -60,7 +60,7 @@
>  #define BTRFS_DISCARD_TARGET_MSEC      (6 * 60 * 60UL * MSEC_PER_SEC)
>  #define BTRFS_DISCARD_MIN_DELAY_MSEC   (1UL)
>  #define BTRFS_DISCARD_MAX_DELAY_MSEC   (1000UL)
> -#define BTRFS_DISCARD_MAX_IOPS         (10U)
> +#define BTRFS_DISCARD_MAX_IOPS         (1000U)
>
>  /* Monotonically decreasing minimum length filters after index 0 */
>  static int discard_minlen[BTRFS_NR_DISCARD_LISTS] = {
> --
> 2.40.0
>

Seems reasonable.

Reviewed-by: Neal Gompa <neal@gompa.dev>


-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH v2 2/2] btrfs: reinterpret async discard iops_limit=0 as no delay

[Neal Gompa]

On Wed, Apr 5, 2023 at 4:00 PM Boris Burkov <boris@bur.io> wrote:
>
> Currently, a limit of 0 results in a hardcoded metering over 6 hours.
> Since the default is a set limit, I suspect no one truly depends on this
> rather arbitrary setting. Repurpose it for an arguably more useful
> "unlimited" mode, where the delay is 0.
>
> Note that if block groups are too new, or go fully empty, there is still
> a delay associated with those conditions. Those delays implement
> heuristics for not trimming a region we are relatively likely to fully
> overwrite soon.
>

For my own clarity, this means that there's a basic heuristic of not
activating right away all the time? If so, that seems sound.

> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>  fs/btrfs/discard.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c
> index 0bc526f5fcd9..c48abc817ed2 100644
> --- a/fs/btrfs/discard.c
> +++ b/fs/btrfs/discard.c
> @@ -56,8 +56,6 @@
>  #define BTRFS_DISCARD_DELAY            (120ULL * NSEC_PER_SEC)
>  #define BTRFS_DISCARD_UNUSED_DELAY     (10ULL * NSEC_PER_SEC)
>
> -/* Target completion latency of discarding all discardable extents */
> -#define BTRFS_DISCARD_TARGET_MSEC      (6 * 60 * 60UL * MSEC_PER_SEC)
>  #define BTRFS_DISCARD_MIN_DELAY_MSEC   (1UL)
>  #define BTRFS_DISCARD_MAX_DELAY_MSEC   (1000UL)
>  #define BTRFS_DISCARD_MAX_IOPS         (1000U)
> @@ -577,6 +575,7 @@ void btrfs_discard_calc_delay(struct btrfs_discard_ctl *discard_ctl)
>         s32 discardable_extents;
>         s64 discardable_bytes;
>         u32 iops_limit;
> +       unsigned long min_delay = BTRFS_DISCARD_MIN_DELAY_MSEC;
>         unsigned long delay;
>
>         discardable_extents = atomic_read(&discard_ctl->discardable_extents);
> @@ -607,13 +606,16 @@ void btrfs_discard_calc_delay(struct btrfs_discard_ctl *discard_ctl)
>         }
>
>         iops_limit = READ_ONCE(discard_ctl->iops_limit);
> -       if (iops_limit)
> +
> +       if (iops_limit) {
>                 delay = MSEC_PER_SEC / iops_limit;
> -       else
> -               delay = BTRFS_DISCARD_TARGET_MSEC / discardable_extents;
> +       } else {
> +               /* unset iops_limit means go as fast as possible, so allow a delay of 0 */
> +               delay = 0;
> +               min_delay = 0;
> +       }
>
> -       delay = clamp(delay, BTRFS_DISCARD_MIN_DELAY_MSEC,
> -                     BTRFS_DISCARD_MAX_DELAY_MSEC);
> +       delay = clamp(delay, min_delay, BTRFS_DISCARD_MAX_DELAY_MSEC);
>         discard_ctl->delay_ms = delay;
>
>         spin_unlock(&discard_ctl->lock);
> --
> 2.40.0
>

LGTM.

Reviewed-by: Neal Gompa <neal@gompa.dev>


-- 
真実はいつも一つ！/ Always, there's only one truth!