[PATCH] btrfs: fix default compression value when set by SETFLAGS ioctl

[PATCH] btrfs: fix default compression value when set by SETFLAGS ioctl

[David Sterba]

The current default for the compression file flag is 'zlib', the zstd
patch silently changed that to zstd. Though the choice of zlib might not
be the best one, we should keep the backward compatibility.

The incompat bit for zstd is not set, so this could lead to a filesystem
with a zstd compression in the extents but no flag for the filesystem.

Fixes: 5c1aab1dd5445ed8bd ("btrfs: Add zstd support")
CC: Nick Terrell <terrelln@fb.com>
Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ioctl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index fd172a93d11a..0ce9a895931a 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -309,10 +309,10 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 
 		if (fs_info->compress_type == BTRFS_COMPRESS_LZO)
 			comp = "lzo";
-		else if (fs_info->compress_type == BTRFS_COMPRESS_ZLIB)
-			comp = "zlib";
-		else
+		else if (fs_info->compress_type == BTRFS_COMPRESS_ZSTD)
 			comp = "zstd";
+		else
+			comp = "zlib";
 		ret = btrfs_set_prop(inode, "btrfs.compression",
 				     comp, strlen(comp), 0);
 		if (ret)
-- 
2.14.0

Re: [PATCH] btrfs: fix default compression value when set by SETFLAGS ioctl

[Nick Terrell]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1453 bytes --]

On 10/31/17, 9:48 AM, "David Sterba" <dsterba@suse.com> wrote:
> The current default for the compression file flag is 'zlib', the zstd
> patch silently changed that to zstd. Though the choice of zlib might not
> be the best one, we should keep the backward compatibility.

Sorry about that, I didn't intentionally change it. I checked over my patch,
and the only other place where an "else" was changed is the other place in
ioctl.c, which looks okay to me.

I'm trying to expose the buggy case in 4.14-rc7. However, since
fs_info->compress_type defaults to ZLIB when -compress is not passed, and
when -compress=no is passed, fs_info->compress_type is not modified, I
don't know how to get fs_info->compress_type to be BTRFS_COMPRESS_NONE.
Is there a way that fs_info->compress_type can be BTRFS_COMPRESS_NONE?

I modified -compress=no to set fs_info->compress_type=BTRFS_COMPRESS_NONE
and the ioctl() call set the compression type to zstd before your patch,
and zlib after, as expected.

Tested-by: Nick Terrell <terrelln@fb.com>

> The incompat bit for zstd is not set, so this could lead to a filesystem
> with a zstd compression in the extents but no flag for the filesystem.
>
> Fixes: 5c1aab1dd5445ed8bd ("btrfs: Add zstd support")
> CC: Nick Terrell <terrelln@fb.com>
> Signed-off-by: David Sterba <dsterba@suse.com>


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±ý»k~ÏâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm

Re: [PATCH] btrfs: fix default compression value when set by SETFLAGS ioctl

[David Sterba]

On Tue, Oct 31, 2017 at 07:21:59PM +0000, Nick Terrell wrote:
> On 10/31/17, 9:48 AM, "David Sterba" <dsterba@suse.com> wrote:
> > The current default for the compression file flag is 'zlib', the zstd
> > patch silently changed that to zstd. Though the choice of zlib might not
> > be the best one, we should keep the backward compatibility.
> 
> Sorry about that, I didn't intentionally change it. I checked over my patch,
> and the only other place where an "else" was changed is the other place in
> ioctl.c, which looks okay to me.
> 
> I'm trying to expose the buggy case in 4.14-rc7. However, since
> fs_info->compress_type defaults to ZLIB when -compress is not passed, and
> when -compress=no is passed, fs_info->compress_type is not modified, I
> don't know how to get fs_info->compress_type to be BTRFS_COMPRESS_NONE.
> Is there a way that fs_info->compress_type can be BTRFS_COMPRESS_NONE?

And it turns out that this is not possible, fs_info::compress_type will
always be one of lzo/zlib/zstd. This is zlib by default, so any chattr +c
will set zlib. In order to set zstd by chattr, there would have to be at
least one mount with zstd or defrag -czstd. And this will set the
incompat bit.

> I modified -compress=no to set fs_info->compress_type=BTRFS_COMPRESS_NONE
> and the ioctl() call set the compression type to zstd before your patch,
> and zlib after, as expected.

I must have assumed that mounting with -o compress=no will reset the
compress_type. So unless I missed something else, the default will not
change and the feature flags match the actual filesystem state. False
alert, sorry. At least I don't have to send a late pull request.