* Re: [PATCH] btrfs: auto enable discard=async when possible
2022-07-27 15:01 [PATCH] btrfs: auto enable discard=async when possible David Sterba
@ 2022-07-27 18:46 ` Boris Burkov
2022-07-27 18:47 ` Roman Mamedov
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Boris Burkov @ 2022-07-27 18:46 UTC (permalink / raw)
To: dsterba, linux-btrfs
On Wed, Jul 27, 2022 at 05:01:58PM +0200, David Sterba wrote:
> There's a request to automatically enable async discard for capable
> devices. We can do that, the async mode is designed to wait for larger
> freed extents and is not intrusive, with limits to iops, kbps or latency.
>
> The status and tunables will be exported in /sys/fs/btrfs/FSID/discard .
>
> The automatic selection is done if there's at least one discard capable
> device in the filesystem (not capable devices are skipped). Mounting
> with any other discard option will honor that option, notably mounting
> with nodiscard will keep it disabled.
>
> Link: https://lore.kernel.org/linux-btrfs/CAEg-Je_b1YtdsCR0zS5XZ_SbvJgN70ezwvRwLiCZgDGLbeMB=w@mail.gmail.com/
> Signed-off-by: David Sterba <dsterba@suse.com>
Works for me, on mount and ro mount.
Reviewed-by: Boris Burkov <boris@bur.io>
> ---
> fs/btrfs/ctree.h | 1 +
> fs/btrfs/disk-io.c | 14 ++++++++++++++
> fs/btrfs/super.c | 2 ++
> fs/btrfs/volumes.c | 3 +++
> fs/btrfs/volumes.h | 2 ++
> 5 files changed, 22 insertions(+)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 4db85b9dc7ed..0a338311f8e2 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1503,6 +1503,7 @@ enum {
> BTRFS_MOUNT_DISCARD_ASYNC = (1UL << 28),
> BTRFS_MOUNT_IGNOREBADROOTS = (1UL << 29),
> BTRFS_MOUNT_IGNOREDATACSUMS = (1UL << 30),
> + BTRFS_MOUNT_NODISCARD = (1UL << 31),
> };
>
> #define BTRFS_DEFAULT_COMMIT_INTERVAL (30)
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 3fac429cf8a4..8f8e33219d4d 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3767,6 +3767,20 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
> btrfs_set_and_info(fs_info, SSD, "enabling ssd optimizations");
> }
>
> + /*
> + * For devices supporting discard turn on discard=async automatically,
> + * unless it's already set or disabled. This could be turned off by
> + * nodiscard for the same mount.
> + */
> + if (!(btrfs_test_opt(fs_info, DISCARD_SYNC) ||
> + btrfs_test_opt(fs_info, DISCARD_ASYNC) ||
> + btrfs_test_opt(fs_info, NODISCARD)) &&
> + fs_info->fs_devices->discardable) {
> + btrfs_set_and_info(fs_info, DISCARD_ASYNC,
> + "auto enabling discard=async");
> + btrfs_clear_opt(fs_info->mount_opt, NODISCARD);
> + }
> +
> /*
> * Mount does not set all options immediately, we can do it now and do
> * not have to wait for transaction commit
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 4c7089b1681b..1032aaa2c2f4 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -915,12 +915,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
> ret = -EINVAL;
> goto out;
> }
> + btrfs_clear_opt(info->mount_opt, NODISCARD);
> break;
> case Opt_nodiscard:
> btrfs_clear_and_info(info, DISCARD_SYNC,
> "turning off discard");
> btrfs_clear_and_info(info, DISCARD_ASYNC,
> "turning off async discard");
> + btrfs_set_opt(info->mount_opt, NODISCARD);
> break;
> case Opt_space_cache:
> case Opt_space_cache_version:
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 272901514b0c..22bfc7806ccb 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -639,6 +639,9 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
> if (!bdev_nonrot(bdev))
> fs_devices->rotating = true;
>
> + if (bdev_max_discard_sectors(bdev))
> + fs_devices->discardable = true;
> +
> device->bdev = bdev;
> clear_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state);
> device->mode = flags;
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 5639961b3626..4c716603449d 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -329,6 +329,8 @@ struct btrfs_fs_devices {
> * nonrot flag set
> */
> bool rotating;
> + /* Devices support TRIM/discard commands */
> + bool discardable;
>
> struct btrfs_fs_info *fs_info;
> /* sysfs kobjects */
> --
> 2.36.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: auto enable discard=async when possible
2022-07-27 15:01 [PATCH] btrfs: auto enable discard=async when possible David Sterba
2022-07-27 18:46 ` Boris Burkov
@ 2022-07-27 18:47 ` Roman Mamedov
2022-07-28 11:57 ` David Sterba
2022-08-06 7:29 ` Neal Gompa
2022-11-06 5:08 ` Wang Yugui
3 siblings, 1 reply; 6+ messages in thread
From: Roman Mamedov @ 2022-07-27 18:47 UTC (permalink / raw)
To: David Sterba; +Cc: linux-btrfs
On Wed, 27 Jul 2022 17:01:58 +0200
David Sterba <dsterba@suse.com> wrote:
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1503,6 +1503,7 @@ enum {
> BTRFS_MOUNT_DISCARD_ASYNC = (1UL << 28),
> BTRFS_MOUNT_IGNOREBADROOTS = (1UL << 29),
> BTRFS_MOUNT_IGNOREDATACSUMS = (1UL << 30),
> + BTRFS_MOUNT_NODISCARD = (1UL << 31),
> };
>
> #define BTRFS_DEFAULT_COMMIT_INTERVAL (30)
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 3fac429cf8a4..8f8e33219d4d 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3767,6 +3767,20 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
> btrfs_set_and_info(fs_info, SSD, "enabling ssd optimizations");
> }
>
> + /*
> + * For devices supporting discard turn on discard=async automatically,
> + * unless it's already set or disabled. This could be turned off by
> + * nodiscard for the same mount.
> + */
> + if (!(btrfs_test_opt(fs_info, DISCARD_SYNC) ||
> + btrfs_test_opt(fs_info, DISCARD_ASYNC) ||
> + btrfs_test_opt(fs_info, NODISCARD)) &&
> + fs_info->fs_devices->discardable) {
> + btrfs_set_and_info(fs_info, DISCARD_ASYNC,
> + "auto enabling discard=async");
> + btrfs_clear_opt(fs_info->mount_opt, NODISCARD);
Spaces are used in the above line instead of a 2nd TAB.
Also I am probably clueless, but it seems the condition just checked that
NODISCARD was not set, so what is the purpose of also clearing it?
Thanks
--
With respect,
Roman
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: auto enable discard=async when possible
2022-07-27 18:47 ` Roman Mamedov
@ 2022-07-28 11:57 ` David Sterba
0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2022-07-28 11:57 UTC (permalink / raw)
To: Roman Mamedov; +Cc: David Sterba, linux-btrfs
On Wed, Jul 27, 2022 at 11:47:58PM +0500, Roman Mamedov wrote:
> On Wed, 27 Jul 2022 17:01:58 +0200
> David Sterba <dsterba@suse.com> wrote:
>
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -1503,6 +1503,7 @@ enum {
> > BTRFS_MOUNT_DISCARD_ASYNC = (1UL << 28),
> > BTRFS_MOUNT_IGNOREBADROOTS = (1UL << 29),
> > BTRFS_MOUNT_IGNOREDATACSUMS = (1UL << 30),
> > + BTRFS_MOUNT_NODISCARD = (1UL << 31),
> > };
> >
> > #define BTRFS_DEFAULT_COMMIT_INTERVAL (30)
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index 3fac429cf8a4..8f8e33219d4d 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -3767,6 +3767,20 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
> > btrfs_set_and_info(fs_info, SSD, "enabling ssd optimizations");
> > }
> >
> > + /*
> > + * For devices supporting discard turn on discard=async automatically,
> > + * unless it's already set or disabled. This could be turned off by
> > + * nodiscard for the same mount.
> > + */
> > + if (!(btrfs_test_opt(fs_info, DISCARD_SYNC) ||
> > + btrfs_test_opt(fs_info, DISCARD_ASYNC) ||
> > + btrfs_test_opt(fs_info, NODISCARD)) &&
> > + fs_info->fs_devices->discardable) {
> > + btrfs_set_and_info(fs_info, DISCARD_ASYNC,
> > + "auto enabling discard=async");
> > + btrfs_clear_opt(fs_info->mount_opt, NODISCARD);
>
> Spaces are used in the above line instead of a 2nd TAB.
>
> Also I am probably clueless, but it seems the condition just checked that
> NODISCARD was not set, so what is the purpose of also clearing it?
I think it's out of habit and consistency with other option handling to
make it clear what's the new state (compression and datacow/datasum), but
you're right that here it's a bit redundant.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: auto enable discard=async when possible
2022-07-27 15:01 [PATCH] btrfs: auto enable discard=async when possible David Sterba
2022-07-27 18:46 ` Boris Burkov
2022-07-27 18:47 ` Roman Mamedov
@ 2022-08-06 7:29 ` Neal Gompa
2022-11-06 5:08 ` Wang Yugui
3 siblings, 0 replies; 6+ messages in thread
From: Neal Gompa @ 2022-08-06 7:29 UTC (permalink / raw)
To: dsterba, linux-btrfs
On Wed, Jul 27, 2022 at 11:28 AM David Sterba <dsterba@suse.com> wrote:
>
> There's a request to automatically enable async discard for capable
> devices. We can do that, the async mode is designed to wait for larger
> freed extents and is not intrusive, with limits to iops, kbps or latency.
>
> The status and tunables will be exported in /sys/fs/btrfs/FSID/discard .
>
> The automatic selection is done if there's at least one discard capable
> device in the filesystem (not capable devices are skipped). Mounting
> with any other discard option will honor that option, notably mounting
> with nodiscard will keep it disabled.
>
> Link: https://lore.kernel.org/linux-btrfs/CAEg-Je_b1YtdsCR0zS5XZ_SbvJgN70ezwvRwLiCZgDGLbeMB=w@mail.gmail.com/
> Signed-off-by: David Sterba <dsterba@suse.com>
Looks great.
Reviewed-by: Neal Gompa <ngompa@fedoraproject.org>
--
Neal Gompa (FAS: ngompa)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: auto enable discard=async when possible
2022-07-27 15:01 [PATCH] btrfs: auto enable discard=async when possible David Sterba
` (2 preceding siblings ...)
2022-08-06 7:29 ` Neal Gompa
@ 2022-11-06 5:08 ` Wang Yugui
3 siblings, 0 replies; 6+ messages in thread
From: Wang Yugui @ 2022-11-06 5:08 UTC (permalink / raw)
To: dsterba, linux-btrfs
Hi,
> There's a request to automatically enable async discard for capable
> devices. We can do that, the async mode is designed to wait for larger
> freed extents and is not intrusive, with limits to iops, kbps or latency.
>
> The status and tunables will be exported in /sys/fs/btrfs/FSID/discard .
>
> The automatic selection is done if there's at least one discard capable
> device in the filesystem (not capable devices are skipped). Mounting
> with any other discard option will honor that option, notably mounting
> with nodiscard will keep it disabled.
>
> Link: https://lore.kernel.org/linux-btrfs/CAEg-Je_b1YtdsCR0zS5XZ_SbvJgN70ezwvRwLiCZgDGLbeMB=w@mail.gmail.com/
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> fs/btrfs/ctree.h | 1 +
> fs/btrfs/disk-io.c | 14 ++++++++++++++
> fs/btrfs/super.c | 2 ++
> fs/btrfs/volumes.c | 3 +++
> fs/btrfs/volumes.h | 2 ++
> 5 files changed, 22 insertions(+)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 4db85b9dc7ed..0a338311f8e2 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1503,6 +1503,7 @@ enum {
> BTRFS_MOUNT_DISCARD_ASYNC = (1UL << 28),
> BTRFS_MOUNT_IGNOREBADROOTS = (1UL << 29),
> BTRFS_MOUNT_IGNOREDATACSUMS = (1UL << 30),
> + BTRFS_MOUNT_NODISCARD = (1UL << 31),
> };
>
> #define BTRFS_DEFAULT_COMMIT_INTERVAL (30)
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 3fac429cf8a4..8f8e33219d4d 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3767,6 +3767,20 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
> btrfs_set_and_info(fs_info, SSD, "enabling ssd optimizations");
> }
>
> + /*
> + * For devices supporting discard turn on discard=async automatically,
> + * unless it's already set or disabled. This could be turned off by
> + * nodiscard for the same mount.
> + */
> + if (!(btrfs_test_opt(fs_info, DISCARD_SYNC) ||
> + btrfs_test_opt(fs_info, DISCARD_ASYNC) ||
> + btrfs_test_opt(fs_info, NODISCARD)) &&
> + fs_info->fs_devices->discardable) {
> + btrfs_set_and_info(fs_info, DISCARD_ASYNC,
> + "auto enabling discard=async");
> + btrfs_clear_opt(fs_info->mount_opt, NODISCARD);
We do not need 'btrfs_clear_opt(fs_info->mount_opt, NODISCARD);' here?
Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2022/11/06
> + }
> +
> /*
> * Mount does not set all options immediately, we can do it now and do
> * not have to wait for transaction commit
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 4c7089b1681b..1032aaa2c2f4 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -915,12 +915,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
> ret = -EINVAL;
> goto out;
> }
> + btrfs_clear_opt(info->mount_opt, NODISCARD);
> break;
> case Opt_nodiscard:
> btrfs_clear_and_info(info, DISCARD_SYNC,
> "turning off discard");
> btrfs_clear_and_info(info, DISCARD_ASYNC,
> "turning off async discard");
> + btrfs_set_opt(info->mount_opt, NODISCARD);
> break;
> case Opt_space_cache:
> case Opt_space_cache_version:
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 272901514b0c..22bfc7806ccb 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -639,6 +639,9 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
> if (!bdev_nonrot(bdev))
> fs_devices->rotating = true;
>
> + if (bdev_max_discard_sectors(bdev))
> + fs_devices->discardable = true;
> +
> device->bdev = bdev;
> clear_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state);
> device->mode = flags;
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 5639961b3626..4c716603449d 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -329,6 +329,8 @@ struct btrfs_fs_devices {
> * nonrot flag set
> */
> bool rotating;
> + /* Devices support TRIM/discard commands */
> + bool discardable;
>
> struct btrfs_fs_info *fs_info;
> /* sysfs kobjects */
> --
> 2.36.1
^ permalink raw reply [flat|nested] 6+ messages in thread