btrfs: avoid misleading talk about "compression level 0"
diff mbox

Message ID 20171021164901.11529-1-kilobyte@angband.pl
State New
Headers show

Commit Message

Adam Borowski Oct. 21, 2017, 4:49 p.m. UTC
Many compressors do assign a meaning to level 0: either null compression or
the lowest possible level.  This differs from our "unset thus default".
Thus, let's not unnecessarily confuse users.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 fs/btrfs/super.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

David Sterba Oct. 25, 2017, 1:23 p.m. UTC | #1
On Sat, Oct 21, 2017 at 06:49:01PM +0200, Adam Borowski wrote:
> Many compressors do assign a meaning to level 0: either null compression or
> the lowest possible level.  This differs from our "unset thus default".
> Thus, let's not unnecessarily confuse users.

I agree 'level 0' confusing, however I'd like to keep the level
mentioned in the message.

We could add

#define	BTRFS_COMPRESSION_ZLIB_DEFAULT		3

and use it in btrfs_compress_str2level.

> 
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> ---
>  fs/btrfs/super.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index f9d4522336db..144fabfbd246 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -551,7 +551,9 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>  			      compress_force != saved_compress_force)) ||
>  			    (!btrfs_test_opt(info, COMPRESS) &&
>  			     no_compress == 1)) {
> -				btrfs_info(info, "%s %s compression, level %d",
> +				btrfs_printk(info, info->compress_level ?
> +					   KERN_INFO"%s %s compression, level %d" :
> +					   KERN_INFO"%s %s compression",

Please keep using btrfs_info, the KERN_INFO prefix would not work here.
btrfs_printk prepends the filesystem description and the message level
must be at the beginning.

>  					   (compress_force) ? "force" : "use",
>  					   compress_type, info->compress_level);
>  			}
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adam Borowski Oct. 25, 2017, 7:16 p.m. UTC | #2
On Wed, Oct 25, 2017 at 03:23:11PM +0200, David Sterba wrote:
> On Sat, Oct 21, 2017 at 06:49:01PM +0200, Adam Borowski wrote:
> > Many compressors do assign a meaning to level 0: either null compression or
> > the lowest possible level.  This differs from our "unset thus default".
> > Thus, let's not unnecessarily confuse users.
> 
> I agree 'level 0' confusing, however I'd like to keep the level
> mentioned in the message.
> 
> We could add
> 
> #define	BTRFS_COMPRESSION_ZLIB_DEFAULT		3
> 
> and use it in btrfs_compress_str2level.

I considered this but every algorithm has a different default, thus we'd
need separate cases for zlib vs zstd, while lzo has no settable level at
all.  Still, this is just some extra lines of code, thus doable.

> > Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> > ---
> >  fs/btrfs/super.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > index f9d4522336db..144fabfbd246 100644
> > --- a/fs/btrfs/super.c
> > +++ b/fs/btrfs/super.c
> > @@ -551,7 +551,9 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
> >  			      compress_force != saved_compress_force)) ||
> >  			    (!btrfs_test_opt(info, COMPRESS) &&
> >  			     no_compress == 1)) {
> > -				btrfs_info(info, "%s %s compression, level %d",
> > +				btrfs_printk(info, info->compress_level ?
> > +					   KERN_INFO"%s %s compression, level %d" :
> > +					   KERN_INFO"%s %s compression",
> 
> Please keep using btrfs_info, the KERN_INFO prefix would not work here.
> btrfs_printk prepends the filesystem description and the message level
> must be at the beginning.

Seems to work for me:
[   14.072575] BTRFS info (device sda1): use lzo compression
with identical colors as other info messages next to it.

But if we're to expand this code, ternary operators would get too hairy,
thus this can go at least for clarity.

> >  					   (compress_force) ? "force" : "use",
> >  					   compress_type, info->compress_level);
> >  			}
>

Patch
diff mbox

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index f9d4522336db..144fabfbd246 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -551,7 +551,9 @@  int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			      compress_force != saved_compress_force)) ||
 			    (!btrfs_test_opt(info, COMPRESS) &&
 			     no_compress == 1)) {
-				btrfs_info(info, "%s %s compression, level %d",
+				btrfs_printk(info, info->compress_level ?
+					   KERN_INFO"%s %s compression, level %d" :
+					   KERN_INFO"%s %s compression",
 					   (compress_force) ? "force" : "use",
 					   compress_type, info->compress_level);
 			}