Re: [PATCH v2] btrfs/defrag: implement compression levels

[David Sterba]

On Wed, Mar 05, 2025 at 11:32:34AM +0100, Daniel Vacek wrote:
> The zstd and zlib compression types support setting compression levels.
> Enhance the defrag interface to specify the levels as well.
> 
> Signed-off-by: Daniel Vacek <neelx@xxxxxxxx>
> Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>
> ---
> v2: Fixed the commit message and added an explicit level range check.

Where is the level range check? It silently clamps the range but this is
not a check. What I had in mind was to return -EINVAL if the level is
out of range.

> @@ -1376,10 +1377,21 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,
>  		return -EINVAL;
>  
>  	if (do_compress) {
> -		if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES)
> -			return -EINVAL;
> -		if (range->compress_type)
> -			compress_type = range->compress_type;
> +		if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL) {
> +			if (range->compress.type >= BTRFS_NR_COMPRESS_TYPES)
> +				return -EINVAL;
> +			if (range->compress.type) {
> +				compress_type = range->compress.type;
> +				compress_level= range->compress.level;

Please put spaces around binary operators, so " = ".

> +				compress_level= btrfs_compress_set_level(compress_type,
> +									 compress_level);

This should check if the test is in the range.

My idea was to add helper like this

bool btrfs_compress_level_valid(type, level) {
	... ops = btrfs_compress_op[type];

	if (level < ops->min_type || level > ops->max_type)
		return false;
}

> +			}
> +		} else {
> +			if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES)
> +				return -EINVAL;
> +			if (range->compress_type)
> +				compress_type = range->compress_type;
> +		}
>  	}
>  
>  	if (extent_thresh == 0)
>  	if (ret)
> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> index d3b222d7af240..3540d33d6f50c 100644
> --- a/include/uapi/linux/btrfs.h
> +++ b/include/uapi/linux/btrfs.h
> @@ -615,7 +615,9 @@ struct btrfs_ioctl_clone_range_args {
>   */
>  #define BTRFS_DEFRAG_RANGE_COMPRESS 1
>  #define BTRFS_DEFRAG_RANGE_START_IO 2
> +#define BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL 4
>  #define BTRFS_DEFRAG_RANGE_FLAGS_SUPP	(BTRFS_DEFRAG_RANGE_COMPRESS |		\
> +					 BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL |	\
>  					 BTRFS_DEFRAG_RANGE_START_IO)
>  
>  struct btrfs_ioctl_defrag_range_args {
> @@ -643,7 +645,13 @@ struct btrfs_ioctl_defrag_range_args {
>  	 * for this defrag operation.  If unspecified, zlib will
>  	 * be used
>  	 */
> -	__u32 compress_type;

Please update the comment mentioning that the type + level are used when
the BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL flag is set.

> +	union {
> +		__u32 compress_type;
> +		struct {
> +			__u8 type;
> +			__s8 level;
> +		} compress;
> +	};
>  
>  	/* spare for later */
>  	__u32 unused[4];
> -- 
> 2.47.2
>