Skip to contentSkip to conversation

btrfs: property: Set incompat flag of lzo/zstd compression

PATCH In review Misono Tomohiro 5 participants 7 messages May 15, 2018 1 message not in the archive Btrfs file system

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Misono Tomohiro

author
#1 lore raw

PATCH 1/1 btrfs: property: Set incompat flag of lzo/zstd compression

Incompat flag of lzo/zstd compression should be set at:
1. mount time (-o compress/compress-force)
2. when defrag is done
3. when property is set

Currently 3. is missing and this commit adds this.

Signed-off-by Tomohiro Misono <[email protected]>
---
 fs/btrfs/props.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
fs/btrfs/props.c +8 -4
index 53a8c95828e3..dc6140013ae8 100644
@@ -380,6 +380,7 @@ static int prop_compression_apply(struct inode *inode,
 				  const char *value,
 				  size_t len)
 {
+	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	int type;
 
 	if (len == 0) {

@@ -390,14 +391,17 @@ static int prop_compression_apply(struct inode *inode,
 		return 0;
 	}
 
-	if (!strncmp("lzo", value, 3))
+	if (!strncmp("lzo", value, 3)) {
 		type = BTRFS_COMPRESS_LZO;
-	else if (!strncmp("zlib", value, 4))
+		btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
+	} else if (!strncmp("zlib", value, 4)) {
 		type = BTRFS_COMPRESS_ZLIB;
-	else if (!strncmp("zstd", value, len))
+	} else if (!strncmp("zstd", value, len)) {
 		type = BTRFS_COMPRESS_ZSTD;
-	else
+		btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
+	} else {
 		return -EINVAL;
+	}
 
 	BTRFS_I(inode)->flags &= ~BTRFS_INODE_NOCOMPRESS;
 	BTRFS_I(inode)->flags |= BTRFS_INODE_COMPRESS;
Signature · 1 line
2.14.3
#2 lore raw
Quoted 7 lines · Misono Tomohiro

Incompat flag of lzo/zstd compression should be set at:
1. mount time (-o compress/compress-force)
2. when defrag is done
3. when property is set

Currently 3. is missing and this commit adds this.

If I don't misunderstand, compression property of an inode is only apply for *the* inode, not the whole filesystem.
So the original logical should be okay.

Thanks,
Su

Quoted 40 lines
Signed-off-by Tomohiro Misono <[email protected]>
---
 fs/btrfs/props.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
fs/btrfs/props.c +8 -4
index 53a8c95828e3..dc6140013ae8 100644
@@ -380,6 +380,7 @@ static int prop_compression_apply(struct inode *inode,
 				  const char *value,
 				  size_t len)
 {
+	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	int type;
 
 	if (len == 0) {

@@ -390,14 +391,17 @@ static int prop_compression_apply(struct inode *inode,
 		return 0;
 	}
 
-	if (!strncmp("lzo", value, 3))
+	if (!strncmp("lzo", value, 3)) {
 		type = BTRFS_COMPRESS_LZO;
-	else if (!strncmp("zlib", value, 4))
+		btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
+	} else if (!strncmp("zlib", value, 4)) {
 		type = BTRFS_COMPRESS_ZLIB;
-	else if (!strncmp("zstd", value, len))
+	} else if (!strncmp("zstd", value, len)) {
 		type = BTRFS_COMPRESS_ZSTD;
-	else
+		btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
+	} else {
 		return -EINVAL;
+	}
 
 	BTRFS_I(inode)->flags &= ~BTRFS_INODE_NOCOMPRESS;
 	BTRFS_I(inode)->flags |= BTRFS_INODE_COMPRESS;
#3 lore raw
Quoted 8 lines · Su Yue
Quoted 5 lines · Misono Tomohiro

Incompat flag of lzo/zstd compression should be set at:
1. mount time (-o compress/compress-force)
2. when defrag is done
3. when property is set

Quoted 4 lines
Quoted 2 lines

Currently 3. is missing and this commit adds this.

If I don't misunderstand, compression property of an inode is only

Embarrassed for bad memory about btrfs_set_fs_incompat(). The patch is fine. Just ignore this thread.

Quoted 48 lines

apply for *the* inode, not the whole filesystem.
So the original logical should be okay.

Thanks,
Su

Quoted 40 lines
Signed-off-by Tomohiro Misono <[email protected]>
---
 fs/btrfs/props.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
fs/btrfs/props.c +8 -4
index 53a8c95828e3..dc6140013ae8 100644
@@ -380,6 +380,7 @@ static int prop_compression_apply(struct inode *inode,
 				  const char *value,
 				  size_t len)
 {
+	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	int type;
 
 	if (len == 0) {

@@ -390,14 +391,17 @@ static int prop_compression_apply(struct inode *inode,
 		return 0;
 	}
 
-	if (!strncmp("lzo", value, 3))
+	if (!strncmp("lzo", value, 3)) {
 		type = BTRFS_COMPRESS_LZO;
-	else if (!strncmp("zlib", value, 4))
+		btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
+	} else if (!strncmp("zlib", value, 4)) {
 		type = BTRFS_COMPRESS_ZLIB;
-	else if (!strncmp("zstd", value, len))
+	} else if (!strncmp("zstd", value, len)) {
 		type = BTRFS_COMPRESS_ZSTD;
-	else
+		btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
+	} else {
 		return -EINVAL;
+	}
 
 	BTRFS_I(inode)->flags &= ~BTRFS_INODE_NOCOMPRESS;
 	BTRFS_I(inode)->flags |= BTRFS_INODE_COMPRESS;
#4 lore raw
In reply to Su Yue

Su Yue posted on Tue, 15 May 2018 16:05:01 +0800 as excerpted:

Quoted 6 lines · Misono Tomohiro
Quoted 4 lines

Incompat flag of lzo/zstd compression should be set at:
1. mount time (-o compress/compress-force)
2. when defrag is done 3. when property is set

Quoted 6 lines
Quoted 3 lines

Currently 3. is missing and this commit adds this.

If I don't misunderstand, compression property of an inode is only apply for *the* inode, not the whole filesystem.
So the original logical should be okay.

But the inode is on the filesystem, and if it's compressed with lzo/zstd, the incompat flag should be set to avoid mounting with an earlier kernel that doesn't understand that compression and would therefore, if we're lucky, simply fail to read the data compressed in that file/inode. (If we're unlucky it could blow up with kernel memory corruption like James Harvey's current case of unexpected, corrupted compressed data in a nocow file that being nocow, doesn't have csum validation to fail and abort the decompression, and shouldn't be compressed at all.)

So better to set the incompat flag and refuse to mount at all on kernels that don't have the required compression support.

Signature · 3 lines
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman
#5 lore raw
Quoted 25 lines · Duncan

Su Yue posted on Tue, 15 May 2018 16:05:01 +0800 as excerpted:

Quoted 6 lines · Misono Tomohiro
Quoted 4 lines

Incompat flag of lzo/zstd compression should be set at:
1. mount time (-o compress/compress-force)
2. when defrag is done 3. when property is set

Quoted 6 lines
Quoted 3 lines

Currently 3. is missing and this commit adds this.

If I don't misunderstand, compression property of an inode is only apply for *the* inode, not the whole filesystem.
So the original logical should be okay.

But the inode is on the filesystem, and if it's compressed with lzo/zstd, the incompat flag should be set to avoid mounting with an earlier kernel that doesn't understand that compression and would therefore, if we're lucky, simply fail to read the data compressed in that file/inode. (If we're unlucky it could blow up with kernel memory corruption like James Harvey's current case of unexpected, corrupted compressed data in a nocow file that being nocow, doesn't have csum validation to fail and abort the decompression, and shouldn't be compressed at all.)

Quoted 3 lines

So better to set the incompat flag and refuse to mount at all on kernels that don't have the required compression support.

Get it.
As your conclusion, it's indeed better to set the incompat flag.

Thanks,
Su

Reviewed-by on v1 1/1
#7 lore raw
In reply to Misono Tomohiro
Quoted 6 lines · Misono Tomohiro

Incompat flag of lzo/zstd compression should be set at:
1. mount time (-o compress/compress-force)
2. when defrag is done
3. when property is set

Currently 3. is missing and this commit adds this.

That was missed during the review, thanks for catching it.

Quoted 1 line
Signed-off-by Tomohiro Misono <[email protected]>
Reviewed-by David Sterba <[email protected]>

For 4.17-rc and for stable 4.14+.