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

在 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 <[email protected]>

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);
>   	}