btrfs: super.c: Set compress_level=0 when using compress=lzo
diff mbox series

Message ID 20200803195501.30528-1-marcos@mpdesouza.com
State New
Headers show
Series
  • btrfs: super.c: Set compress_level=0 when using compress=lzo
Related show

Commit Message

Marcos Paulo de Souza Aug. 3, 2020, 7:55 p.m. UTC
From: Marcos Paulo de Souza <mpdesouza@suse.com>

Currently a user can set mount "-o compress" which will set the
compression algorithm to zlib, and use the default compress level for
zlib (3):

relatime,compress=zlib:3,space_cache

If the user remounts the fs using "-o compress=lzo", then the old
compress_level is used:

relatime,compress=lzo:3,space_cache

But lzo does not exposes any tunable compress level. The same happens if we set
any compress argument with different level, even with zstd.

Fix this issue by resetting the compress_level when compress=lzo is specified.
With the fix applied, lzo is shown without compress level, even after remounting
from zlib or zstd:

relatime,compress=lzo,space_cache

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---

 I found this issue while testing mount options. Should we tag this fix for
 stable since the introduction of the compress_level, or is it no worthy?

 fs/btrfs/super.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

David Sterba Aug. 11, 2020, 2:12 p.m. UTC | #1
On Mon, Aug 03, 2020 at 04:55:01PM -0300, Marcos Paulo de Souza wrote:
> From: Marcos Paulo de Souza <mpdesouza@suse.com>
> 
> Currently a user can set mount "-o compress" which will set the
> compression algorithm to zlib, and use the default compress level for
> zlib (3):
> 
> relatime,compress=zlib:3,space_cache
> 
> If the user remounts the fs using "-o compress=lzo", then the old
> compress_level is used:
> 
> relatime,compress=lzo:3,space_cache
> 
> But lzo does not exposes any tunable compress level. The same happens if we set
> any compress argument with different level, even with zstd.
> 
> Fix this issue by resetting the compress_level when compress=lzo is specified.
> With the fix applied, lzo is shown without compress level, even after remounting
> from zlib or zstd:
> 
> relatime,compress=lzo,space_cache
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
> 
>  I found this issue while testing mount options. Should we tag this fix for
>  stable since the introduction of the compress_level, or is it no worthy?

I'll tag it for stable, it's user-visible and confusing. Thanks.
David Sterba Aug. 11, 2020, 2:22 p.m. UTC | #2
On Mon, Aug 03, 2020 at 04:55:01PM -0300, Marcos Paulo de Souza wrote:
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -614,6 +614,8 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>  			} else if (strncmp(args[0].from, "lzo", 3) == 0) {
>  				compress_type = "lzo";
>  				info->compress_type = BTRFS_COMPRESS_LZO;
> +				/* lzo does not expose compression levels */
> +				info->compress_level = 0;

I had that as a fixup in patches fixing the other message errors, with
info->compress_level = 1.

With level 0 it'll print

	btrfs_info(info, "%s %s compression, level %d",
		   (compress_force) ? "force" : "use",
		   compress_type, info->compress_level);


	"use lzo compression, level 0"

Which might be confusing as well, but super.c:btrfs_show_options will
not print the level in that case so I think it's fine with 0.

Patch
diff mbox series

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index a034ed52b5f6..dd2f05f9d6c6 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -614,6 +614,8 @@  int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			} else if (strncmp(args[0].from, "lzo", 3) == 0) {
 				compress_type = "lzo";
 				info->compress_type = BTRFS_COMPRESS_LZO;
+				/* lzo does not expose compression levels */
+				info->compress_level = 0;
 				btrfs_set_opt(info->mount_opt, COMPRESS);
 				btrfs_clear_opt(info->mount_opt, NODATACOW);
 				btrfs_clear_opt(info->mount_opt, NODATASUM);