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

在 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)) {