[PATCH v2 0/4] btrfs: add per-inode compression levels in xattrs

[PATCH v2 0/4] btrfs: add per-inode compression levels in xattrs

[koraynilay]

Add per-inode compression levels using
btrfs property set /path/to/file compression "algo:level", using the
same syntax as the compress mount option
If set on folders, all new children will inherit the setting, while
already existing children will be unaffected.

This patch series also fixes a small "bug": before, when setting
btrfs.compression, it would keep the mount option level, even if the
algo was different (!!), so with e.g. compress=zstd:15 and
btrfs.compression=zlib the data gets compressed at zlib:9 (because it
would still get clamped at the right range); now if the level isn't
explicitly set it will use the default level for the algo.

I also wanna thank Zygo for helping me by explaining stuff and for
noticing this "bug".

koraynilay (4):
  btrfs: export btrfs_match_compress_type(), move it to compression.h
  btrfs: also validate compression levels in
    btrfs_compress_is_valid_type()
  btrfs: add per-inode compression levels in xattrs
  btrfs: support inheritance for per-inode compression levels

 fs/btrfs/btrfs_inode.h |  1 +
 fs/btrfs/compression.c | 28 ++++++++++++++++++---
 fs/btrfs/compression.h |  3 ++-
 fs/btrfs/inode.c       |  2 ++
 fs/btrfs/props.c       | 57 +++++++++++++++++++++++++++++++++---------
 fs/btrfs/super.c       |  8 ------
 6 files changed, 75 insertions(+), 24 deletions(-)


base-commit: a59f57e2aa127c5354168d2ec4bac920df1be4f4
-- 
2.55.0

[PATCH v2 1/4] btrfs: export btrfs_match_compress_type(), move it to compression.h

[koraynilay]

Export btrfs_match_compress_type() from being a static and locally used
function in super.c and move it to compression.c, exporting it in
compression.h.  This allows it to be used in the next patches as
validation for compression algorithm names.

Assisted-by: Gemini:3.1-pro antigravity-cli-1.1.5
Signed-off-by: koraynilay <koray.fra@gmail.com>
---
 fs/btrfs/compression.c | 17 +++++++++++++++++
 fs/btrfs/compression.h |  1 +
 fs/btrfs/super.c       |  8 --------
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index ffb6b52863a7..58138f300b58 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -1660,3 +1660,20 @@ int btrfs_compress_str2level(unsigned int type, const char *str, int *level_ret)
 	*level_ret = btrfs_compress_set_level(type, level);
 	return 0;
 }
+
+/**
+ * btrfs_match_compress_type - Check if the string matches the compression type
+ * @string:         The string to check
+ * @type:           The compression type string (name) to match against (e.g. "zstd")
+ * @may_have_level: If true, the string may have a level suffix (e.g., ":1")
+ *
+ * Return: %true if the string matches the type and, if %may_have_level is %true,
+ * has a level suffix, %false otherwise.
+ */
+bool btrfs_match_compress_type(const char *string, const char *type, bool may_have_level)
+{
+	const int len = strlen(type);
+
+	return (strncmp(string, type, len) == 0) &&
+		((may_have_level && string[len] == ':') || string[len] == '\0');
+}
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index 1022dc53ec51..e67ba47b4cdc 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -97,6 +97,7 @@ void btrfs_submit_compressed_write(struct btrfs_ordered_extent *ordered,
 void btrfs_submit_compressed_read(struct btrfs_bio *bbio);
 
 int btrfs_compress_str2level(unsigned int type, const char *str, int *level_ret);
+bool btrfs_match_compress_type(const char *string, const char *type, bool may_have_level);
 
 struct folio *btrfs_alloc_compr_folio(struct btrfs_fs_info *fs_info, gfp_t gfp);
 void btrfs_free_compr_folio(struct folio *folio);
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index f4e34898d581..7a21085c33c5 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -264,14 +264,6 @@ static const struct fs_parameter_spec btrfs_fs_parameters[] = {
 	{}
 };
 
-static bool btrfs_match_compress_type(const char *string, const char *type, bool may_have_level)
-{
-	const int len = strlen(type);
-
-	return (strncmp(string, type, len) == 0) &&
-		((may_have_level && string[len] == ':') || string[len] == '\0');
-}
-
 static int btrfs_parse_compress(struct btrfs_fs_context *ctx,
 				const struct fs_parameter *param, int opt)
 {
-- 
2.55.0

[PATCH v2 2/4] btrfs: also validate compression levels in btrfs_compress_is_valid_type()

[koraynilay]

Change btrfs_compress_is_valid_type() to use btrfs_match_compress_type()
and btrfs_compress_str2level() instead of a simple strncmp, which would
validate even incorrect strings (e.g. "zstd:invalid" or "zstdinvalid").

This also makes the function validate levels in the same way the
compress= option gets parsed, allowing bigger or smaller values, but
still clamping them to the min or max supported.

Furthermore, remove the len parameter, since now it requires a
NUL-terminated string because of btrfs_compress_str2level(); this change
is fine because btrfs_compress_is_valid_type() is used only once in
props.c

Assisted-by: Gemini:3.1-pro antigravity-cli-1.1.5
Signed-off-by: koraynilay <koray.fra@gmail.com>
---
 fs/btrfs/compression.c | 11 ++++++++---
 fs/btrfs/compression.h |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 58138f300b58..833c5f45afec 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -70,18 +70,23 @@ static struct compressed_bio *alloc_compressed_bio(struct btrfs_inode *inode,
 	return to_compressed_bio(bbio);
 }
 
-bool btrfs_compress_is_valid_type(const char *str, size_t len)
+bool btrfs_compress_is_valid_type(const char *str)
 {
+	size_t len = strlen(str);
 	int i;
 
 	for (i = 1; i < ARRAY_SIZE(btrfs_compress_types); i++) {
 		size_t comp_len = strlen(btrfs_compress_types[i]);
+		const char *comp_type = btrfs_compress_types[i];
+		int tmp_level;
 
 		if (len < comp_len)
 			continue;
 
-		if (!strncmp(btrfs_compress_types[i], str, comp_len))
-			return true;
+		if (btrfs_match_compress_type(str, comp_type, true)) {
+			if (btrfs_compress_str2level(i, str + comp_len, &tmp_level) == 0)
+				return true;
+		}
 	}
 	return false;
 }
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index e67ba47b4cdc..c63bed9f4152 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -132,7 +132,7 @@ extern const struct btrfs_compress_levels btrfs_lzo_compress;
 extern const struct btrfs_compress_levels btrfs_zstd_compress;
 
 const char* btrfs_compress_type2str(enum btrfs_compression_type type);
-bool btrfs_compress_is_valid_type(const char *str, size_t len);
+bool btrfs_compress_is_valid_type(const char *str);
 
 int btrfs_compress_heuristic(struct btrfs_inode *inode, u64 start, u64 end);
 
-- 
2.55.0

[PATCH v2 3/4] btrfs: add per-inode compression levels in xattrs

[koraynilay]

Add support for specifying algo:level using
btrfs property set /path/to/file compression "algo:level".

Add a signed 8 bit prop_compress_level property to btrfs_inode, which
can support from level -128 to 127, plenty for the currently supported
algo:level(s).

Change prop_compression_apply() to use the already present
btrfs_match_compress_type() and btrfs_compress_str2level() to parse the
xattr, using kmemdup_nul() to convert the user-supplied xattr value to a
NUL-terminated string to be used by btrfs_compress_str2level(). The
same approach was taken in prop_compression_validate() for
btrfs_compress_is_valid_type().

Assisted-by: Gemini:3.1-pro antigravity-cli-1.1.5
Signed-off-by: koraynilay <koray.fra@gmail.com>
---
 fs/btrfs/btrfs_inode.h |  1 +
 fs/btrfs/inode.c       |  2 ++
 fs/btrfs/props.c       | 43 +++++++++++++++++++++++++++++++++---------
 3 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index 1082fa92c145..0a4e567d0109 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -130,6 +130,7 @@ struct btrfs_inode {
 
 	/* Cached value of inode property 'compression'. */
 	u8 prop_compress;
+	s8 prop_compress_level;
 
 	/*
 	 * Force compression on the file using the defrag ioctl, could be
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 2534cd9284d5..cff4b3e97559 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -938,6 +938,7 @@ static void compress_file_range(struct btrfs_work *work)
 		compress_level = inode->defrag_compress_level;
 	} else if (inode->prop_compress) {
 		compress_type = inode->prop_compress;
+		compress_level = inode->prop_compress_level;
 	}
 
 	/* Compression level is applied here. */
@@ -2326,6 +2327,7 @@ static int run_delalloc_inline(struct btrfs_inode *inode, struct folio *locked_f
 			compress_level = inode->defrag_compress_level;
 		} else if (inode->prop_compress) {
 			compress_type = inode->prop_compress;
+			compress_level = inode->prop_compress_level;
 		}
 		cb = btrfs_compress_bio(inode, 0, blocksize, compress_type, compress_level, 0);
 		if (IS_ERR(cb)) {
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index bb77d46376d4..f79a61a2759f 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -295,20 +295,28 @@ int btrfs_load_inode_props(struct btrfs_inode *inode, struct btrfs_path *path)
 static int prop_compression_validate(const struct btrfs_inode *inode,
 				     const char *value, size_t len)
 {
+	int ret = -EINVAL;
+	const char *value_str;
+
 	if (!btrfs_inode_can_compress(inode))
 		return -EINVAL;
 
 	if (!value)
 		return 0;
 
-	if (btrfs_compress_is_valid_type(value, len))
-		return 0;
-
 	if ((len == 2 && strncmp("no", value, 2) == 0) ||
 	    (len == 4 && strncmp("none", value, 4) == 0))
 		return 0;
 
-	return -EINVAL;
+	value_str = kmemdup_nul(value, len, GFP_KERNEL);
+	if (!value_str)
+		return -ENOMEM;
+
+	if (btrfs_compress_is_valid_type(value_str))
+		ret = 0;
+
+	kfree(value_str);
+	return ret;
 }
 
 static int prop_compression_apply(struct btrfs_inode *inode, const char *value,
@@ -316,6 +324,9 @@ static int prop_compression_apply(struct btrfs_inode *inode, const char *value,
 {
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
 	int type;
+	int level = 0;
+	int ret = 0;
+	const char *value_str;
 
 	/* Reset to defaults */
 	if (len == 0) {
@@ -335,23 +346,37 @@ static int prop_compression_apply(struct btrfs_inode *inode, const char *value,
 		return 0;
 	}
 
-	if (!strncmp("lzo", value, 3)) {
+	value_str = kmemdup_nul(value, len, GFP_KERNEL);
+	if (!value_str)
+		return -ENOMEM;
+
+	if (btrfs_match_compress_type(value_str, "lzo", true)) {
 		type = BTRFS_COMPRESS_LZO;
 		btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
-	} else if (!strncmp("zlib", value, 4)) {
+	} else if (btrfs_match_compress_type(value_str, "zlib", true)) {
 		type = BTRFS_COMPRESS_ZLIB;
-	} else if (!strncmp("zstd", value, 4)) {
+		ret = btrfs_compress_str2level(type, value_str + 4, &level);
+		if (ret < 0)
+			goto out;
+	} else if (btrfs_match_compress_type(value_str, "zstd", true)) {
 		type = BTRFS_COMPRESS_ZSTD;
+		ret = btrfs_compress_str2level(type, value_str + 4, &level);
+		if (ret < 0)
+			goto out;
 		btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
 	} else {
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
 
 	inode->flags &= ~BTRFS_INODE_NOCOMPRESS;
 	inode->flags |= BTRFS_INODE_COMPRESS;
 	inode->prop_compress = type;
+	inode->prop_compress_level = level;
 
-	return 0;
+out:
+	kfree(value_str);
+	return ret;
 }
 
 static bool prop_compression_ignore(const struct btrfs_inode *inode)
-- 
2.55.0

[PATCH v2 4/4] btrfs: support inheritance for per-inode compression levels

[koraynilay]

Change prop_handler's extract() signature to take an output buffer and
its length. This allows for the algo:level string to be generated
dynamically, but now the caller is in charge of managing that buffer
memory. Before this patch prop_compression_extract() would return only
the address of statically compiled string without being able to add the
level, now it concatenates the level to that statically compiled string.

This signature change is fine because the only currently supported prop
is compression, so there is only 1 extract() function.

Assisted-by: Gemini:3.1-pro antigravity-cli-1.1.5
Signed-off-by: koraynilay <koray.fra@gmail.com>
---
 fs/btrfs/props.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index f79a61a2759f..9c97792800fb 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -27,7 +27,7 @@ struct prop_handler {
 	int (*validate)(const struct btrfs_inode *inode, const char *value,
 			size_t len);
 	int (*apply)(struct btrfs_inode *inode, const char *value, size_t len);
-	const char *(*extract)(const struct btrfs_inode *inode);
+	const char *(*extract)(const struct btrfs_inode *inode, char *buf, size_t len);
 	bool (*ignore)(const struct btrfs_inode *inode);
 	int inheritable;
 };
@@ -395,12 +395,19 @@ static bool prop_compression_ignore(const struct btrfs_inode *inode)
 	return false;
 }
 
-static const char *prop_compression_extract(const struct btrfs_inode *inode)
+static const char *prop_compression_extract(const struct btrfs_inode *inode,
+					    char *buf, size_t len)
 {
 	switch (inode->prop_compress) {
 	case BTRFS_COMPRESS_ZLIB:
 	case BTRFS_COMPRESS_LZO:
 	case BTRFS_COMPRESS_ZSTD:
+		if (inode->prop_compress_level) {
+			snprintf(buf, len, "%s:%d",
+				 btrfs_compress_type2str(inode->prop_compress),
+				 inode->prop_compress_level);
+			return buf;
+		}
 		return btrfs_compress_type2str(inode->prop_compress);
 	default:
 		break;
@@ -437,6 +444,7 @@ int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans,
 		const struct prop_handler *h = &prop_handlers[i];
 		const char *value;
 		u64 num_bytes = 0;
+		char buf[16];
 
 		if (!h->inheritable)
 			continue;
@@ -444,7 +452,7 @@ int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans,
 		if (h->ignore(inode))
 			continue;
 
-		value = h->extract(parent);
+		value = h->extract(parent, buf, sizeof(buf));
 		if (!value)
 			continue;
 
-- 
2.55.0

Re: [PATCH v2 2/4] btrfs: also validate compression levels in btrfs_compress_is_valid_type()

[Qu Wenruo]

在 2026/8/9 11:20, koraynilay 写道:
> Change btrfs_compress_is_valid_type() to use btrfs_match_compress_type()
> and btrfs_compress_str2level() instead of a simple strncmp, which would
> validate even incorrect strings (e.g. "zstd:invalid" or "zstdinvalid").
> 
> This also makes the function validate levels in the same way the
> compress= option gets parsed, allowing bigger or smaller values, but
> still clamping them to the min or max supported.
> 
> Furthermore, remove the len parameter, since now it requires a
> NUL-terminated string because of btrfs_compress_str2level(); this change
> is fine because btrfs_compress_is_valid_type() is used only once in
> props.c
> 
> Assisted-by: Gemini:3.1-pro antigravity-cli-1.1.5
> Signed-off-by: koraynilay <koray.fra@gmail.com>
> ---
>   fs/btrfs/compression.c | 11 ++++++++---
>   fs/btrfs/compression.h |  2 +-
>   2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 58138f300b58..833c5f45afec 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -70,18 +70,23 @@ static struct compressed_bio *alloc_compressed_bio(struct btrfs_inode *inode,
>   	return to_compressed_bio(bbio);
>   }
>   
> -bool btrfs_compress_is_valid_type(const char *str, size_t len)
> +bool btrfs_compress_is_valid_type(const char *str)

Please make sure every commit compiles, this will easily break bisection.

You're changing a function prototype and implementation without 
modifying any callers, this fails compiling.

Furthermore, I do not think it's a good idea to just rely on the strlen().

E.g. if a crafted image removing the last terminating NUL, relying 
strlen() can easily go beyond the expected string.

I think the change to remove @len is going to reduce the robustness of 
the original code.

Thanks,
Qu

>   {
> +	size_t len = strlen(str);
>   	int i;
>   
>   	for (i = 1; i < ARRAY_SIZE(btrfs_compress_types); i++) {
>   		size_t comp_len = strlen(btrfs_compress_types[i]);
> +		const char *comp_type = btrfs_compress_types[i];
> +		int tmp_level;
>   
>   		if (len < comp_len)
>   			continue;
>   
> -		if (!strncmp(btrfs_compress_types[i], str, comp_len))
> -			return true;
> +		if (btrfs_match_compress_type(str, comp_type, true)) {
> +			if (btrfs_compress_str2level(i, str + comp_len, &tmp_level) == 0)
> +				return true;
> +		}
>   	}
>   	return false;
>   }
> diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> index e67ba47b4cdc..c63bed9f4152 100644
> --- a/fs/btrfs/compression.h
> +++ b/fs/btrfs/compression.h
> @@ -132,7 +132,7 @@ extern const struct btrfs_compress_levels btrfs_lzo_compress;
>   extern const struct btrfs_compress_levels btrfs_zstd_compress;
>   
>   const char* btrfs_compress_type2str(enum btrfs_compression_type type);
> -bool btrfs_compress_is_valid_type(const char *str, size_t len);
> +bool btrfs_compress_is_valid_type(const char *str);
>   
>   int btrfs_compress_heuristic(struct btrfs_inode *inode, u64 start, u64 end);
>   

Re: [PATCH v2 3/4] btrfs: add per-inode compression levels in xattrs

[Qu Wenruo]

在 2026/8/9 11:20, koraynilay 写道:
> Add support for specifying algo:level using
> btrfs property set /path/to/file compression "algo:level".
> 
> Add a signed 8 bit prop_compress_level property to btrfs_inode, which
> can support from level -128 to 127, plenty for the currently supported
> algo:level(s).
> 
> Change prop_compression_apply() to use the already present
> btrfs_match_compress_type() and btrfs_compress_str2level() to parse the
> xattr, using kmemdup_nul() to convert the user-supplied xattr value to a
> NUL-terminated string to be used by btrfs_compress_str2level(). The
> same approach was taken in prop_compression_validate() for
> btrfs_compress_is_valid_type().
> 
> Assisted-by: Gemini:3.1-pro antigravity-cli-1.1.5
> Signed-off-by: koraynilay <koray.fra@gmail.com>
> ---
>   fs/btrfs/btrfs_inode.h |  1 +
>   fs/btrfs/inode.c       |  2 ++
>   fs/btrfs/props.c       | 43 +++++++++++++++++++++++++++++++++---------
>   3 files changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index 1082fa92c145..0a4e567d0109 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -130,6 +130,7 @@ struct btrfs_inode {
>   
>   	/* Cached value of inode property 'compression'. */
>   	u8 prop_compress;
> +	s8 prop_compress_level;
>   
>   	/*
>   	 * Force compression on the file using the defrag ioctl, could be
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 2534cd9284d5..cff4b3e97559 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -938,6 +938,7 @@ static void compress_file_range(struct btrfs_work *work)
>   		compress_level = inode->defrag_compress_level;
>   	} else if (inode->prop_compress) {
>   		compress_type = inode->prop_compress;
> +		compress_level = inode->prop_compress_level;

I'd prefer to have a dedicated patch to set compress_level to the 
default value 0, as a proper bug fix as the first patch of the series.

As you mentioned in the cover-letter, this is in fact fixing a bug in 
the old behavior (mismatched algo and level).

So it's definitely worth a dedicated fix, so that we can backport the 
fix without pulling in the full series for older kernels.

Thanks,
Qu

>   	}
>   
>   	/* Compression level is applied here. */
> @@ -2326,6 +2327,7 @@ static int run_delalloc_inline(struct btrfs_inode *inode, struct folio *locked_f
>   			compress_level = inode->defrag_compress_level;
>   		} else if (inode->prop_compress) {
>   			compress_type = inode->prop_compress;
> +			compress_level = inode->prop_compress_level;
>   		}
>   		cb = btrfs_compress_bio(inode, 0, blocksize, compress_type, compress_level, 0);
>   		if (IS_ERR(cb)) {
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index bb77d46376d4..f79a61a2759f 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -295,20 +295,28 @@ int btrfs_load_inode_props(struct btrfs_inode *inode, struct btrfs_path *path)
>   static int prop_compression_validate(const struct btrfs_inode *inode,
>   				     const char *value, size_t len)
>   {
> +	int ret = -EINVAL;
> +	const char *value_str;
> +
>   	if (!btrfs_inode_can_compress(inode))
>   		return -EINVAL;
>   
>   	if (!value)
>   		return 0;
>   
> -	if (btrfs_compress_is_valid_type(value, len))
> -		return 0;
> -
>   	if ((len == 2 && strncmp("no", value, 2) == 0) ||
>   	    (len == 4 && strncmp("none", value, 4) == 0))
>   		return 0;
>   
> -	return -EINVAL;
> +	value_str = kmemdup_nul(value, len, GFP_KERNEL);
> +	if (!value_str)
> +		return -ENOMEM;
> +
> +	if (btrfs_compress_is_valid_type(value_str))
> +		ret = 0;
> +
> +	kfree(value_str);
> +	return ret;
>   }
>   
>   static int prop_compression_apply(struct btrfs_inode *inode, const char *value,
> @@ -316,6 +324,9 @@ static int prop_compression_apply(struct btrfs_inode *inode, const char *value,
>   {
>   	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>   	int type;
> +	int level = 0;
> +	int ret = 0;
> +	const char *value_str;
>   
>   	/* Reset to defaults */
>   	if (len == 0) {
> @@ -335,23 +346,37 @@ static int prop_compression_apply(struct btrfs_inode *inode, const char *value,
>   		return 0;
>   	}
>   
> -	if (!strncmp("lzo", value, 3)) {
> +	value_str = kmemdup_nul(value, len, GFP_KERNEL);
> +	if (!value_str)
> +		return -ENOMEM;
> +
> +	if (btrfs_match_compress_type(value_str, "lzo", true)) {
>   		type = BTRFS_COMPRESS_LZO;
>   		btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
> -	} else if (!strncmp("zlib", value, 4)) {
> +	} else if (btrfs_match_compress_type(value_str, "zlib", true)) {
>   		type = BTRFS_COMPRESS_ZLIB;
> -	} else if (!strncmp("zstd", value, 4)) {
> +		ret = btrfs_compress_str2level(type, value_str + 4, &level);
> +		if (ret < 0)
> +			goto out;
> +	} else if (btrfs_match_compress_type(value_str, "zstd", true)) {
>   		type = BTRFS_COMPRESS_ZSTD;
> +		ret = btrfs_compress_str2level(type, value_str + 4, &level);
> +		if (ret < 0)
> +			goto out;
>   		btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
>   	} else {
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto out;
>   	}
>   
>   	inode->flags &= ~BTRFS_INODE_NOCOMPRESS;
>   	inode->flags |= BTRFS_INODE_COMPRESS;
>   	inode->prop_compress = type;
> +	inode->prop_compress_level = level;
>   
> -	return 0;
> +out:
> +	kfree(value_str);
> +	return ret;
>   }
>   
>   static bool prop_compression_ignore(const struct btrfs_inode *inode)

Re: [PATCH v2 4/4] btrfs: support inheritance for per-inode compression levels

[Qu Wenruo]

在 2026/8/9 11:20, koraynilay 写道:
> Change prop_handler's extract() signature to take an output buffer and
> its length. This allows for the algo:level string to be generated
> dynamically, but now the caller is in charge of managing that buffer
> memory. Before this patch prop_compression_extract() would return only
> the address of statically compiled string without being able to add the
> level, now it concatenates the level to that statically compiled string.
> 
> This signature change is fine because the only currently supported prop
> is compression, so there is only 1 extract() function.
> 
> Assisted-by: Gemini:3.1-pro antigravity-cli-1.1.5
> Signed-off-by: koraynilay <koray.fra@gmail.com>
> ---
>   fs/btrfs/props.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index f79a61a2759f..9c97792800fb 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -27,7 +27,7 @@ struct prop_handler {
>   	int (*validate)(const struct btrfs_inode *inode, const char *value,
>   			size_t len);
>   	int (*apply)(struct btrfs_inode *inode, const char *value, size_t len);
> -	const char *(*extract)(const struct btrfs_inode *inode);
> +	const char *(*extract)(const struct btrfs_inode *inode, char *buf, size_t len);
>   	bool (*ignore)(const struct btrfs_inode *inode);
>   	int inheritable;
>   };
> @@ -395,12 +395,19 @@ static bool prop_compression_ignore(const struct btrfs_inode *inode)
>   	return false;
>   }
>   
> -static const char *prop_compression_extract(const struct btrfs_inode *inode)
> +static const char *prop_compression_extract(const struct btrfs_inode *inode,
> +					    char *buf, size_t len)
>   {
>   	switch (inode->prop_compress) {
>   	case BTRFS_COMPRESS_ZLIB:
>   	case BTRFS_COMPRESS_LZO:
>   	case BTRFS_COMPRESS_ZSTD:
> +		if (inode->prop_compress_level) {
> +			snprintf(buf, len, "%s:%d",
> +				 btrfs_compress_type2str(inode->prop_compress),
> +				 inode->prop_compress_level);
> +			return buf;
> +		}
>   		return btrfs_compress_type2str(inode->prop_compress);
>   	default:
>   		break;
> @@ -437,6 +444,7 @@ int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans,
>   		const struct prop_handler *h = &prop_handlers[i];
>   		const char *value;
>   		u64 num_bytes = 0;
> +		char buf[16];

Please use a macro to define the value.

In a perfect world, we want some way to determine the max string size at 
compile time, comparing the max length of "zlib", "zstd", "lzo", "none" 
with extra ":" and possible level values, but we do not have an easy way 
to do that.

So a comment on the how the size is calculated would be enough, e.g. the 
maximum length is "zstd:-15" with terminating 0, and round it up to 
power of 2 for future expansion.

Thanks,
Qu
>   
>   		if (!h->inheritable)
>   			continue;
> @@ -444,7 +452,7 @@ int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans,
>   		if (h->ignore(inode))
>   			continue;
>   
> -		value = h->extract(parent);
> +		value = h->extract(parent, buf, sizeof(buf));
>   		if (!value)
>   			continue;
>   

Re: [PATCH v2 2/4] btrfs: also validate compression levels in btrfs_compress_is_valid_type()

[koraynilay]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

On Sun Aug 9, 2026 at 4:53 AM CEST, Qu Wenruo wrote:
> Please make sure every commit compiles, this will easily break bisection.
>
> You're changing a function prototype and implementation without 
> modifying any callers, this fails compiling.

Right, I'm sorry, I didn't notice I changed the caller in 3/4 as that
call point also had other changes.

> Furthermore, I do not think it's a good idea to just rely on the strlen().
>
> E.g. if a crafted image removing the last terminating NUL, relying 
> strlen() can easily go beyond the expected string.
>
> I think the change to remove @len is going to reduce the robustness of 
> the original code.

That was what I feared too, the problem is that
btrfs_compress_str2level() uses kstrtoint(), which requires the string
to be null-terminated[1], so the len paramenter would only be used by
if (len < comp_len) and that reduces the robustness even more IMO, since
a developer could think it's safe because of len, while it actually
isn't because of kstrtoint().
The only fix I can think of would be to use
_parse_integer_limit(..., len) directly or write a wrapper.
Or just document that it needs a null-terminated string.

Thanks.

Best,
koraynilay

[1]: 
from lib/kstrtox.c:251:
 * kstrtoint - convert a string to an int
 * @s: The start of the string. The string must be null-terminated [...]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 3/4] btrfs: add per-inode compression levels in xattrs

[koraynilay]

[-- Attachment #1: Type: text/plain, Size: 2745 bytes --]

> I'd prefer to have a dedicated patch to set compress_level to the 
> default value 0, as a proper bug fix as the first patch of the series.
>
> As you mentioned in the cover-letter, this is in fact fixing a bug in 
> the old behavior (mismatched algo and level).
>
> So it's definitely worth a dedicated fix, so that we can backport the 
> fix without pulling in the full series for older kernels.

Ah yes of course, although, would it be better to have a single patch
that looks like this[2], which would also need to add
prop_compress_level's initialization to 0 as per [1] (that would
probably be good anyway) or simply a:

 compress_type = inode->prop_compress;
+compress_level = 0; //level in xattr isn't supported yet

in the 2 relevant places?

Thanks.

Best,
koraynilay

P.S. I also noticed that defrag_compress_level too doesn't get
initialized to 0, so if we choose that approach a separate patch that
adds it would be in order IMO.

[1]:
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e057aceaa68d..aec977f47cfb 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7896,7 +7896,8 @@ struct inode *btrfs_alloc_inode(struct super_block *sb)
                                              BTRFS_BLOCK_RSV_DELALLOC);
        ei->runtime_flags = 0;
        ei->prop_compress = BTRFS_COMPRESS_NONE;
+       ei->prop_compress_level = 0;
        ei->defrag_compress = BTRFS_COMPRESS_NONE;
 
        ei->delayed_node = NULL;

[2]:
diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index 1082fa92c145..0a4e567d0109 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -130,6 +130,7 @@ struct btrfs_inode {
  
  	/* Cached value of inode property 'compression'. */
  	u8 prop_compress;
+	s8 prop_compress_level;
  
  	/*
  	 * Force compression on the file using the defrag ioctl, could be
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 2534cd9284d5..cff4b3e97559 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -938,6 +938,7 @@ static void compress_file_range(struct btrfs_work *work)
  		compress_level = inode->defrag_compress_level;
  	} else if (inode->prop_compress) {
  		compress_type = inode->prop_compress;
+		compress_level = inode->prop_compress_level;
  	}
  
  	/* Compression level is applied here. */
@@ -2326,6 +2327,7 @@ static int run_delalloc_inline(struct btrfs_inode *inode, struct folio *locked_f
  			compress_level = inode->defrag_compress_level;
  		} else if (inode->prop_compress) {
  			compress_type = inode->prop_compress;
+			compress_level = inode->prop_compress_level;
  		}
  		cb = btrfs_compress_bio(inode, 0, blocksize, compress_type, compress_level, 0);
  		if (IS_ERR(cb)) {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 3/4] btrfs: add per-inode compression levels in xattrs

[Qu Wenruo]

在 2026/8/9 13:20, koraynilay 写道:
>> I'd prefer to have a dedicated patch to set compress_level to the
>> default value 0, as a proper bug fix as the first patch of the series.
>>
>> As you mentioned in the cover-letter, this is in fact fixing a bug in
>> the old behavior (mismatched algo and level).
>>
>> So it's definitely worth a dedicated fix, so that we can backport the
>> fix without pulling in the full series for older kernels.
> 
> Ah yes of course, although, would it be better to have a single patch
> that looks like this[2], which would also need to add
> prop_compress_level's initialization to 0 as per [1] (that would
> probably be good anyway) or simply a:
> 
>   compress_type = inode->prop_compress;
> +compress_level = 0; //level in xattr isn't supported yet
> 
> in the 2 relevant places?

Personally speaking, I'd prefer only introduce prop_compress_level when 
we really need it, aka, only when we introduce the extra level in the XATTR.

So I'd prefer the fix to be as simple as the following one.

Thanks,
Qu

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 50c6640543b9..bea2d1409fac 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -942,6 +942,7 @@ static void compress_file_range(struct btrfs_work *work)
                 compress_level = inode->defrag_compress_level;
         } else if (inode->prop_compress) {
                 compress_type = inode->prop_compress;
+               compress_level = 0;
         }

         /* Compression level is applied here. */
@@ -2338,6 +2339,7 @@ static int run_delalloc_inline(struct btrfs_inode 
*inode, struct folio *locked_f
                         compress_level = inode->defrag_compress_level;
                 } else if (inode->prop_compress) {
                         compress_type = inode->prop_compress;
+                       compress_level = 0;
                 }
                 cb = btrfs_compress_bio(inode, 0, blocksize, 
compress_type, compress_level, 0);
                 if (IS_ERR(cb)) {



> 
> Thanks.
> 
> Best,
> koraynilay
> 
> P.S. I also noticed that defrag_compress_level too doesn't get
> initialized to 0, so if we choose that approach a separate patch that
> adds it would be in order IMO.
> 
> [1]:
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index e057aceaa68d..aec977f47cfb 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7896,7 +7896,8 @@ struct inode *btrfs_alloc_inode(struct super_block *sb)
>                                                BTRFS_BLOCK_RSV_DELALLOC);
>          ei->runtime_flags = 0;
>          ei->prop_compress = BTRFS_COMPRESS_NONE;
> +       ei->prop_compress_level = 0;
>          ei->defrag_compress = BTRFS_COMPRESS_NONE;
>   
>          ei->delayed_node = NULL;
> 
> [2]:
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index 1082fa92c145..0a4e567d0109 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -130,6 +130,7 @@ struct btrfs_inode {
>    
>    	/* Cached value of inode property 'compression'. */
>    	u8 prop_compress;
> +	s8 prop_compress_level;
>    
>    	/*
>    	 * Force compression on the file using the defrag ioctl, could be
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 2534cd9284d5..cff4b3e97559 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -938,6 +938,7 @@ static void compress_file_range(struct btrfs_work *work)
>    		compress_level = inode->defrag_compress_level;
>    	} else if (inode->prop_compress) {
>    		compress_type = inode->prop_compress;
> +		compress_level = inode->prop_compress_level;
>    	}
>    
>    	/* Compression level is applied here. */
> @@ -2326,6 +2327,7 @@ static int run_delalloc_inline(struct btrfs_inode *inode, struct folio *locked_f
>    			compress_level = inode->defrag_compress_level;
>    		} else if (inode->prop_compress) {
>    			compress_type = inode->prop_compress;
> +			compress_level = inode->prop_compress_level;
>    		}
>    		cb = btrfs_compress_bio(inode, 0, blocksize, compress_type, compress_level, 0);
>    		if (IS_ERR(cb)) {

Re: [PATCH v2 2/4] btrfs: also validate compression levels in btrfs_compress_is_valid_type()

[Qu Wenruo]

在 2026/8/9 13:05, koraynilay 写道:
> On Sun Aug 9, 2026 at 4:53 AM CEST, Qu Wenruo wrote:
>> Please make sure every commit compiles, this will easily break bisection.
>>
>> You're changing a function prototype and implementation without
>> modifying any callers, this fails compiling.
> 
> Right, I'm sorry, I didn't notice I changed the caller in 3/4 as that
> call point also had other changes.
> 
>> Furthermore, I do not think it's a good idea to just rely on the strlen().
>>
>> E.g. if a crafted image removing the last terminating NUL, relying
>> strlen() can easily go beyond the expected string.
>>
>> I think the change to remove @len is going to reduce the robustness of
>> the original code.
> 
> That was what I feared too, the problem is that
> btrfs_compress_str2level() uses kstrtoint(), which requires the string
> to be null-terminated[1], so the len paramenter would only be used by
> if (len < comp_len) and that reduces the robustness even more IMO, since
> a developer could think it's safe because of len, while it actually
> isn't because of kstrtoint().
> The only fix I can think of would be to use
> _parse_integer_limit(..., len) directly or write a wrapper.
> Or just document that it needs a null-terminated string.

OK, you're right, in that case we're not losing anything.

So feel free to go ahead just fixing the compiling error.

Thanks,
Qu

> 
> Thanks.
> 
> Best,
> koraynilay
> 
> [1]:
> from lib/kstrtox.c:251:
>   * kstrtoint - convert a string to an int
>   * @s: The start of the string. The string must be null-terminated [...]

Re: [PATCH v2 3/4] btrfs: add per-inode compression levels in xattrs

[koraynilay]

[-- Attachment #1: Type: text/plain, Size: 2942 bytes --]

> I'd prefer to have a dedicated patch to set compress_level to the 
> default value 0, as a proper bug fix as the first patch of the series.
>
> As you mentioned in the cover-letter, this is in fact fixing a bug in 
> the old behavior (mismatched algo and level).
>
> So it's definitely worth a dedicated fix, so that we can backport the 
> fix without pulling in the full series for older kernels.

Ok so, after thinking about it more and bouncing ideas around (with Zygo
too) I realized one thing, that while it *is* technically a bug, I don't
think it's a bug worth backporting.

My reasoning is simply that there is no use-case where the current
"buggy" behaviour would be damaging, as the levels get clamped to the
supported range anyway, while arguably there are (albeit very rare and
probably not very smart in the first place) use-cases where fixing it
could be somewhat (limitedly) damaging.

More importantly IMO, doing this would allow us to explicitly explain
the currently undocumented behaviour in the btrfs-property(8) manpage as
"just so you know, for kernel versions < 7.x cross-algo level leakage
from -o compress was happening".



As for how to handle it after having support for levels in the XATTR,
option 2, aka leaking the compress level only if the algo matches, would
be the best imo:

Example use-case:

- /fs has various types of files, from media to git repos, that would
  benefit from the normal compress mount option;
- /fs also has big virtual machine disks, that have very compressible
  parts but also very uncompressible parts;

using only `mount -o compress=zstd:7 /fs` may mark the vm disks with
NOCOMPRESS as soon as an incompressible extent gets found, but setting
btrfs.compression=zstd won't, as it will try to compress every extent
anyway[1].
This way if the user intends to change the compress level for the whole
fs, they can just change the mount option, knowing that the new level
will apply to the (new) vm extents too, like it will for all other files.

In this example "btrfs.compression=zstd" and "btrfs.compression=zstd:0"
would behave the same, which means the file's extent will get compressed
with zstd:7, but when the user remounts with e.g. zstd:15, they will use
this new level (only for the extents written from that point afterwards,
of course).

(I'm ignoring the case where the user wants to change the algorithm and
let the vms inherit it, as for this specific use-case that would likely
require a whole new feature/property to say "try to compress anyway but
not as much as compress-force" and probably most people use zstd anyway
nowadays).



After coming to this conclusion, I'm personally pretty satisfied with
this solution, while I wasn't as much with the other ones.

Thanks again.

Best,
koraynilay

[1]: https://github.com/kdave/btrfs-progs/pull/1152/commits/7ae9e2aa7a35af5e7b656424957a558a9d0dd676

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 3/4] btrfs: add per-inode compression levels in xattrs

[Qu Wenruo]

在 2026/8/11 12:25, koraynilay 写道:
>> I'd prefer to have a dedicated patch to set compress_level to the
>> default value 0, as a proper bug fix as the first patch of the series.
>>
>> As you mentioned in the cover-letter, this is in fact fixing a bug in
>> the old behavior (mismatched algo and level).
>>
>> So it's definitely worth a dedicated fix, so that we can backport the
>> fix without pulling in the full series for older kernels.
> 
> Ok so, after thinking about it more and bouncing ideas around (with Zygo
> too) I realized one thing, that while it *is* technically a bug, I don't
> think it's a bug worth backporting.
> 
> My reasoning is simply that there is no use-case where the current
> "buggy" behaviour would be damaging, as the levels get clamped to the
> supported range anyway, while arguably there are (albeit very rare and
> probably not very smart in the first place) use-cases where fixing it
> could be somewhat (limitedly) damaging.

To be honest, if the current behavior is not damaging, which I agree, 
then it's also not damaging to use the default level.

After all, it's just a level change, which is never damaging.

> 
> More importantly IMO, doing this would allow us to explicitly explain
> the currently undocumented behaviour in the btrfs-property(8) manpage as
> "just so you know, for kernel versions < 7.x cross-algo level leakage
> from -o compress was happening".

Which also applies to option 3.

> 
> 
> 
> As for how to handle it after having support for levels in the XATTR,
> option 2, aka leaking the compress level only if the algo matches, would
> be the best imo:
> 
> Example use-case:
> 
> - /fs has various types of files, from media to git repos, that would
>    benefit from the normal compress mount option;
> - /fs also has big virtual machine disks, that have very compressible
>    parts but also very uncompressible parts;
> 
> using only `mount -o compress=zstd:7 /fs` may mark the vm disks with
> NOCOMPRESS as soon as an incompressible extent gets found, but setting
> btrfs.compression=zstd won't, as it will try to compress every extent
> anyway[1].

BTW, the default level is 3, so 7 is already trying to compress harder 
than default.
(At least from the official man page)

> This way if the user intends to change the compress level for the whole
> fs, they can just change the mount option, knowing that the new level
> will apply to the (new) vm extents too, like it will for all other files.
> 
> In this example "btrfs.compression=zstd" and "btrfs.compression=zstd:0"
> would behave the same, which means the file's extent will get compressed
> with zstd:7, but when the user remounts with e.g. zstd:15, they will use
> this new level (only for the extents written from that point afterwards,
> of course).

I'd say, in this particular case, user should specify a different level 
for VM images, after the level support in XATTR, other than relying on 
the global mount option level.

> 
> (I'm ignoring the case where the user wants to change the algorithm and
> let the vms inherit it, as for this specific use-case that would likely
> require a whole new feature/property to say "try to compress anyway but
> not as much as compress-force" and probably most people use zstd anyway
> nowadays).
> 
> 
> 
> After coming to this conclusion, I'm personally pretty satisfied with
> this solution, while I wasn't as much with the other ones.

Since my idea is pretty different on option 2 vs 3, and I do not find we 
can persuade each other, so I'll leave David to do the final call.

> 
> Thanks again.
> 
> Best,
> koraynilay
> 
> [1]: https://github.com/kdave/btrfs-progs/pull/1152/commits/7ae9e2aa7a35af5e7b656424957a558a9d0dd676

Re: [PATCH v2 3/4] btrfs: add per-inode compression levels in xattrs

[koraynilay]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

On Tue Aug 11, 2026 at 5:21 AM CEST, Qu Wenruo wrote:
>> 
>> More importantly IMO, doing this would allow us to explicitly explain
>> the currently undocumented behaviour in the btrfs-property(8) manpage as
>> "just so you know, for kernel versions < 7.x cross-algo level leakage
>> from -o compress was happening".
>
> Which also applies to option 3.

Option 3 would mean the behaviour can be different depending on the
kernel < 7.X having the backported patch or not tho, so we can't say
that with 100% certainty in the manpage, but it would need to also say
"this is true only if you don't have this specific bugfix patch, if your
kernel has it, then the behaviour is this other one".

(Sorry but I'll reply to the other points tomorrow, I'm too tired to
think about more difficult sentences right now :D)

Thanks.

Best,
koraynilay

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 3/4] btrfs: add per-inode compression levels in xattrs

[koraynilay]

[-- Attachment #1: Type: text/plain, Size: 5067 bytes --]

On Tue Aug 11, 2026 at 5:21 AM CEST, Qu Wenruo wrote:
>
>
> 在 2026/8/11 12:25, koraynilay 写道:
>>> I'd prefer to have a dedicated patch to set compress_level to the
>>> default value 0, as a proper bug fix as the first patch of the series.
>>>
>>> As you mentioned in the cover-letter, this is in fact fixing a bug in
>>> the old behavior (mismatched algo and level).
>>>
>>> So it's definitely worth a dedicated fix, so that we can backport the
>>> fix without pulling in the full series for older kernels.
>> 
>> Ok so, after thinking about it more and bouncing ideas around (with Zygo
>> too) I realized one thing, that while it *is* technically a bug, I don't
>> think it's a bug worth backporting.
>> 
>> My reasoning is simply that there is no use-case where the current
>> "buggy" behaviour would be damaging, as the levels get clamped to the
>> supported range anyway, while arguably there are (albeit very rare and
>> probably not very smart in the first place) use-cases where fixing it
>> could be somewhat (limitedly) damaging.
>
> To be honest, if the current behavior is not damaging, which I agree, 
> then it's also not damaging to use the default level.
>
> After all, it's just a level change, which is never damaging.

Yeah, I meant that while both aren't damaging, option 3 would be a
*little* more damaging vs option 1 (as per https://xkcd.com/1172), since
with option 1 nothing can break at all (because no change would be made
on older kernels).
(For this I mean only the bugfix of course).

>> 
>> More importantly IMO, doing this would allow us to explicitly explain
>> the currently undocumented behaviour in the btrfs-property(8) manpage as
>> "just so you know, for kernel versions < 7.x cross-algo level leakage
>> from -o compress was happening".
>
> Which also applies to option 3.

Option 3 would mean the behaviour can be different depending on the
kernel < 7.X having the backported patch or not tho, so we can't say
that with 100% certainty in the manpage, but it would need to also say
"this is true only if you don't have this specific bugfix patch, if your
kernel has it, then the behaviour is this other one".

>> 
>> 
>> 
>> As for how to handle it after having support for levels in the XATTR,
>> option 2, aka leaking the compress level only if the algo matches, would
>> be the best imo:
>> 
>> Example use-case:
>> 
>> - /fs has various types of files, from media to git repos, that would
>>    benefit from the normal compress mount option;
>> - /fs also has big virtual machine disks, that have very compressible
>>    parts but also very uncompressible parts;
>> 
>> using only `mount -o compress=zstd:7 /fs` may mark the vm disks with
>> NOCOMPRESS as soon as an incompressible extent gets found, but setting
>> btrfs.compression=zstd won't, as it will try to compress every extent
>> anyway[1].
>
> BTW, the default level is 3, so 7 is already trying to compress harder 
> than default.
> (At least from the official man page)
>

Yes, I could've used 15 too, it was just an example of a non-default
level specified in the compress option that would then get kept for
specific inodes that have the property set without any level specified.

>> This way if the user intends to change the compress level for the whole
>> fs, they can just change the mount option, knowing that the new level
>> will apply to the (new) vm extents too, like it will for all other files.
>> 
>> In this example "btrfs.compression=zstd" and "btrfs.compression=zstd:0"
>> would behave the same, which means the file's extent will get compressed
>> with zstd:7, but when the user remounts with e.g. zstd:15, they will use
>> this new level (only for the extents written from that point afterwards,
>> of course).
>
> I'd say, in this particular case, user should specify a different level 
> for VM images, after the level support in XATTR, other than relying on 
> the global mount option level.
>

But if the user wants to have that data be compressed in the same way as
the rest of the fs, they'd have to change the XATTR every time they want
to change the level.

>> 
>> (I'm ignoring the case where the user wants to change the algorithm and
>> let the vms inherit it, as for this specific use-case that would likely
>> require a whole new feature/property to say "try to compress anyway but
>> not as much as compress-force" and probably most people use zstd anyway
>> nowadays).
>> 
>> 
>> 
>> After coming to this conclusion, I'm personally pretty satisfied with
>> this solution, while I wasn't as much with the other ones.
>
> Since my idea is pretty different on option 2 vs 3, and I do not find we 
> can persuade each other, so I'll leave David to do the final call.
>

That's fair, at this point I thought about this so much all options have
pros and cons and I switched from preferring 3, to preferring 2, to now
preferring 1 a bit more (especially because of the docs clarity).

Thanks

Best,
koraynilay

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]