[PATCH] btrfs: preserve the compression property when other inode flags change

[PATCH] btrfs: preserve the compression property when other inode flags change

[Sam Ho]

Setting the compression property on an inode also sets BTRFS_INODE_COMPRESS
on it, and btrfs_inode_flags_to_fsflags() reports that back as FS_COMPR_FL
to FS_IOC_GETFLAGS. chattr(1), like any other FS_IOC_SETFLAGS caller, reads
the current flags, flips only the bit the user asked for and writes the
whole set back, so a request as unrelated as "chattr +i" reaches
btrfs_fileattr_set() with FS_COMPR_FL set.

btrfs_fileattr_set() takes that as a request to enable compression and
overwrites the compression property with the algorithm from the mount
options, falling back to zlib when the filesystem was not mounted with
-o compress. The algorithm the user selected is silently replaced:

  # btrfs property set /mnt/foo compression zstd
  # btrfs property get /mnt/foo compression
  compression=zstd
  # chattr +i /mnt/foo
  # btrfs property get /mnt/foo compression
  compression=zlib

Every chattr operation triggers this, not just +i, and directories are
affected as well, so files created afterwards inherit the wrong algorithm
too. On a filesystem mounted with -o compress=lzo the property is replaced
with lzo instead. Recovering needs a chattr -i first, because the immutable
flag rejects the setxattr that "btrfs property set" issues.

Only pick the default algorithm when compression is actually being enabled
by this call, that is when FS_COMPR_FL was not set before, and otherwise
keep the algorithm recorded in the property. Inodes that have the compress
flag set but no property still get the default, so they behave as before.

Signed-off-by: Sam Ho <samho@synology.com>
---
 fs/btrfs/ioctl.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index baa645e98812..2e54694f06f7 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -384,9 +384,25 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
 		inode_flags |= BTRFS_INODE_COMPRESS;
 		inode_flags &= ~BTRFS_INODE_NOCOMPRESS;
 
-		comp = btrfs_compress_type2str(fs_info->compress_type);
-		if (!comp || comp[0] == 0)
-			comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
+		/*
+		 * If compression was already enabled, keep the algorithm that
+		 * is recorded in the compression property. Otherwise changing
+		 * an unrelated attribute would reset it to the mount default,
+		 * since FS_IOC_SETFLAGS callers pass back the whole flag set
+		 * they got from FS_IOC_GETFLAGS.
+		 *
+		 * Fall back to the default when compression is being enabled
+		 * by this call, or when the inode has the compress flag set
+		 * but no property, which is possible on filesystems touched by
+		 * kernels that did not keep the two in sync.
+		 */
+		if (old_fsflags & FS_COMPR_FL)
+			comp = btrfs_compress_type2str(inode->prop_compress);
+		if (!comp || comp[0] == 0) {
+			comp = btrfs_compress_type2str(fs_info->compress_type);
+			if (!comp || comp[0] == 0)
+				comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
+		}
 	} else {
 		inode_flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS);
 	}
-- 
2.34.1


Disclaimer: The contents of this e-mail message and any attachments are confidential and are intended solely for addressee. The information may also be legally privileged. This transmission is sent in trust, for the sole purpose of delivery to the intended recipient. If you have received this transmission in error, any use, reproduction or dissemination of this transmission is strictly prohibited. If you are not the intended recipient, please immediately notify the sender by reply e-mail or phone and delete this message and its attachments, if any.

Re: [PATCH] btrfs: preserve the compression property when other inode flags change

[Qu Wenruo]

在 2026/8/14 14:44, Sam Ho 写道:
> Setting the compression property on an inode also sets BTRFS_INODE_COMPRESS
> on it, and btrfs_inode_flags_to_fsflags() reports that back as FS_COMPR_FL
> to FS_IOC_GETFLAGS. chattr(1), like any other FS_IOC_SETFLAGS caller, reads
> the current flags, flips only the bit the user asked for and writes the
> whole set back, so a request as unrelated as "chattr +i" reaches
> btrfs_fileattr_set() with FS_COMPR_FL set.
> 
> btrfs_fileattr_set() takes that as a request to enable compression and
> overwrites the compression property with the algorithm from the mount
> options, falling back to zlib when the filesystem was not mounted with
> -o compress. The algorithm the user selected is silently replaced:
> 
>    # btrfs property set /mnt/foo compression zstd
>    # btrfs property get /mnt/foo compression
>    compression=zstd
>    # chattr +i /mnt/foo
>    # btrfs property get /mnt/foo compression
>    compression=zlib
> 
> Every chattr operation triggers this, not just +i, and directories are
> affected as well, so files created afterwards inherit the wrong algorithm
> too. On a filesystem mounted with -o compress=lzo the property is replaced
> with lzo instead. Recovering needs a chattr -i first, because the immutable
> flag rejects the setxattr that "btrfs property set" issues.
> 
> Only pick the default algorithm when compression is actually being enabled
> by this call, that is when FS_COMPR_FL was not set before, and otherwise
> keep the algorithm recorded in the property. Inodes that have the compress
> flag set but no property still get the default, so they behave as before.
> 
> Signed-off-by: Sam Ho <samho@synology.com>

The analyze looks good to me.

Although a minor nitpick related to the compression checks.

> ---
>   fs/btrfs/ioctl.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index baa645e98812..2e54694f06f7 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -384,9 +384,25 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
>   		inode_flags |= BTRFS_INODE_COMPRESS;
>   		inode_flags &= ~BTRFS_INODE_NOCOMPRESS;
>   
> -		comp = btrfs_compress_type2str(fs_info->compress_type);
> -		if (!comp || comp[0] == 0)
> -			comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
> +		/*
> +		 * If compression was already enabled, keep the algorithm that
> +		 * is recorded in the compression property. Otherwise changing
> +		 * an unrelated attribute would reset it to the mount default,
> +		 * since FS_IOC_SETFLAGS callers pass back the whole flag set
> +		 * they got from FS_IOC_GETFLAGS.
> +		 *
> +		 * Fall back to the default when compression is being enabled
> +		 * by this call, or when the inode has the compress flag set
> +		 * but no property, which is possible on filesystems touched by
> +		 * kernels that did not keep the two in sync.
> +		 */
> +		if (old_fsflags & FS_COMPR_FL)
> +			comp = btrfs_compress_type2str(inode->prop_compress);

I do not think we need to always use the string.

We can directly use the compression type and convert it to string at the 
last second.

And we can skip the old_fsflags check and directly check 
inode->prop_compress.

E.g. something like the following will be a little easier to read:

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index ebfb258161c8..befc0df0d5ab 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -384,6 +384,7 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
                 inode_flags &= ~BTRFS_INODE_COMPRESS;
                 inode_flags |= BTRFS_INODE_NOCOMPRESS;
         } else if (fsflags & FS_COMPR_FL) {
+               enum btrfs_compression_type comp_type;

                 if (IS_SWAPFILE(&inode->vfs_inode))
                         return -ETXTBSY;
@@ -391,9 +392,13 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
                 inode_flags |= BTRFS_INODE_COMPRESS;
                 inode_flags &= ~BTRFS_INODE_NOCOMPRESS;

-               comp = btrfs_compress_type2str(fs_info->compress_type);
-               if (!comp || comp[0] == 0)
-                       comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
+               if (inode->prop_compress)
+                       comp_type = inode->prop_compress;
+               else if (fs_info->compress_type)
+                       comp_type = fs_info->compress_type;
+               else
+                       comp_type = BTRFS_COMPRESS_ZLIB;
+               comp = btrfs_compress_type2str(comp_type);
         } else {
                 inode_flags &= ~(BTRFS_INODE_COMPRESS | 
BTRFS_INODE_NOCOMPRESS);
         }

Thanks,
Qu

> +		if (!comp || comp[0] == 0) {
> +			comp = btrfs_compress_type2str(fs_info->compress_type);
> +			if (!comp || comp[0] == 0)
> +				comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
> +		}
>   	} else {
>   		inode_flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS);
>   	}

[PATCH v2] btrfs: preserve the compression property when other inode flags change

[Sam Ho]

Setting the compression property on an inode also sets BTRFS_INODE_COMPRESS
on it, and btrfs_inode_flags_to_fsflags() reports that back as FS_COMPR_FL
to FS_IOC_GETFLAGS. chattr(1), like any other FS_IOC_SETFLAGS caller, reads
the current flags, flips only the bit the user asked for and writes the
whole set back, so a request as unrelated as "chattr +i" reaches
btrfs_fileattr_set() with FS_COMPR_FL set.

btrfs_fileattr_set() takes that as a request to enable compression and
overwrites the compression property with the algorithm from the mount
options, falling back to zlib when the filesystem was not mounted with
-o compress. The algorithm the user selected is silently replaced:

  # btrfs property set /mnt/foo compression zstd
  # btrfs property get /mnt/foo compression
  compression=zstd
  # chattr +i /mnt/foo
  # btrfs property get /mnt/foo compression
  compression=zlib

Every chattr operation triggers this, not just +i, and directories are
affected as well, so files created afterwards inherit the wrong algorithm
too. On a filesystem mounted with -o compress=lzo the property is replaced
with lzo instead. Recovering needs a chattr -i first, because the immutable
flag rejects the setxattr that "btrfs property set" issues.

Prefer the algorithm recorded in the compression property and only fall
back to the mount default when there is no property, so that unrelated
flag changes no longer overwrite the user's choice. Inodes that have the
compress flag set but no property still get the default, so they behave
as before.

Signed-off-by: Sam Ho <samho@synology.com>
---
v2:
 - Pick the compression type first and convert it to a string only once at
   the end, instead of going through btrfs_compress_type2str() for every
   candidate, and check inode->prop_compress directly rather than testing
   old_fsflags for FS_COMPR_FL. Suggested by Qu Wenruo; the two are
   equivalent, since prop_compression_apply() only leaves prop_compress
   set while BTRFS_INODE_COMPRESS is set.
 - Reword the last changelog paragraph and the comment to match.

v1: https://lore.kernel.org/linux-btrfs/20260814051406.1244006-1-samho@synology.com/

 fs/btrfs/ioctl.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index baa645e98812..343d089aa5c3 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -377,6 +377,7 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
 		inode_flags &= ~BTRFS_INODE_COMPRESS;
 		inode_flags |= BTRFS_INODE_NOCOMPRESS;
 	} else if (fsflags & FS_COMPR_FL) {
+		enum btrfs_compression_type comp_type;
 
 		if (IS_SWAPFILE(&inode->vfs_inode))
 			return -ETXTBSY;
@@ -384,9 +385,23 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
 		inode_flags |= BTRFS_INODE_COMPRESS;
 		inode_flags &= ~BTRFS_INODE_NOCOMPRESS;
 
-		comp = btrfs_compress_type2str(fs_info->compress_type);
-		if (!comp || comp[0] == 0)
-			comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
+		/*
+		 * Keep the algorithm recorded in the compression property,
+		 * otherwise changing an unrelated attribute would reset it to
+		 * the mount default, since FS_IOC_SETFLAGS callers write back
+		 * the whole flag set they got from FS_IOC_GETFLAGS and that
+		 * includes FS_COMPR_FL for any inode carrying the property.
+		 *
+		 * Inodes with the compress flag set but no property keep using
+		 * the mount default, so they behave as before.
+		 */
+		if (inode->prop_compress)
+			comp_type = inode->prop_compress;
+		else if (fs_info->compress_type)
+			comp_type = fs_info->compress_type;
+		else
+			comp_type = BTRFS_COMPRESS_ZLIB;
+		comp = btrfs_compress_type2str(comp_type);
 	} else {
 		inode_flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS);
 	}
-- 
2.34.1


Disclaimer: The contents of this e-mail message and any attachments are confidential and are intended solely for addressee. The information may also be legally privileged. This transmission is sent in trust, for the sole purpose of delivery to the intended recipient. If you have received this transmission in error, any use, reproduction or dissemination of this transmission is strictly prohibited. If you are not the intended recipient, please immediately notify the sender by reply e-mail or phone and delete this message and its attachments, if any.

Re: [PATCH v2] btrfs: preserve the compression property when other inode flags change

[Qu Wenruo]

在 2026/8/14 22:31, Sam Ho 写道:
> Setting the compression property on an inode also sets BTRFS_INODE_COMPRESS
> on it, and btrfs_inode_flags_to_fsflags() reports that back as FS_COMPR_FL
> to FS_IOC_GETFLAGS. chattr(1), like any other FS_IOC_SETFLAGS caller, reads
> the current flags, flips only the bit the user asked for and writes the
> whole set back, so a request as unrelated as "chattr +i" reaches
> btrfs_fileattr_set() with FS_COMPR_FL set.
> 
> btrfs_fileattr_set() takes that as a request to enable compression and
> overwrites the compression property with the algorithm from the mount
> options, falling back to zlib when the filesystem was not mounted with
> -o compress. The algorithm the user selected is silently replaced:
> 
>    # btrfs property set /mnt/foo compression zstd
>    # btrfs property get /mnt/foo compression
>    compression=zstd
>    # chattr +i /mnt/foo
>    # btrfs property get /mnt/foo compression
>    compression=zlib
> 
> Every chattr operation triggers this, not just +i, and directories are
> affected as well, so files created afterwards inherit the wrong algorithm
> too. On a filesystem mounted with -o compress=lzo the property is replaced
> with lzo instead. Recovering needs a chattr -i first, because the immutable
> flag rejects the setxattr that "btrfs property set" issues.
> 
> Prefer the algorithm recorded in the compression property and only fall
> back to the mount default when there is no property, so that unrelated
> flag changes no longer overwrite the user's choice. Inodes that have the
> compress flag set but no property still get the default, so they behave
> as before.
> 
> Signed-off-by: Sam Ho <samho@synology.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
> v2:
>   - Pick the compression type first and convert it to a string only once at
>     the end, instead of going through btrfs_compress_type2str() for every
>     candidate, and check inode->prop_compress directly rather than testing
>     old_fsflags for FS_COMPR_FL. Suggested by Qu Wenruo; the two are
>     equivalent, since prop_compression_apply() only leaves prop_compress
>     set while BTRFS_INODE_COMPRESS is set.
>   - Reword the last changelog paragraph and the comment to match.
> 
> v1: https://lore.kernel.org/linux-btrfs/20260814051406.1244006-1-samho@synology.com/
> 
>   fs/btrfs/ioctl.c | 21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index baa645e98812..343d089aa5c3 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -377,6 +377,7 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
>   		inode_flags &= ~BTRFS_INODE_COMPRESS;
>   		inode_flags |= BTRFS_INODE_NOCOMPRESS;
>   	} else if (fsflags & FS_COMPR_FL) {
> +		enum btrfs_compression_type comp_type;
>   
>   		if (IS_SWAPFILE(&inode->vfs_inode))
>   			return -ETXTBSY;
> @@ -384,9 +385,23 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
>   		inode_flags |= BTRFS_INODE_COMPRESS;
>   		inode_flags &= ~BTRFS_INODE_NOCOMPRESS;
>   
> -		comp = btrfs_compress_type2str(fs_info->compress_type);
> -		if (!comp || comp[0] == 0)
> -			comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
> +		/*
> +		 * Keep the algorithm recorded in the compression property,
> +		 * otherwise changing an unrelated attribute would reset it to
> +		 * the mount default, since FS_IOC_SETFLAGS callers write back
> +		 * the whole flag set they got from FS_IOC_GETFLAGS and that
> +		 * includes FS_COMPR_FL for any inode carrying the property.
> +		 *
> +		 * Inodes with the compress flag set but no property keep using
> +		 * the mount default, so they behave as before.
> +		 */
> +		if (inode->prop_compress)
> +			comp_type = inode->prop_compress;
> +		else if (fs_info->compress_type)
> +			comp_type = fs_info->compress_type;
> +		else
> +			comp_type = BTRFS_COMPRESS_ZLIB;
> +		comp = btrfs_compress_type2str(comp_type);
>   	} else {
>   		inode_flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS);
>   	}