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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

The feature itself looks good to me.

Although not sure if a blank commit message is fine for this case.

在 2025/3/5 03:44, Daniel Vacek 写道:
> Signed-off-by: Daniel Vacek <neelx@suse.com>
> ---
>   fs/btrfs/btrfs_inode.h     |  2 ++
>   fs/btrfs/defrag.c          | 22 +++++++++++++++++-----
>   fs/btrfs/fs.h              |  2 +-
>   fs/btrfs/inode.c           | 10 +++++++---
>   include/uapi/linux/btrfs.h | 10 +++++++++-
>   5 files changed, 36 insertions(+), 10 deletions(-)
>
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index aa1f55cd81b79..5ee9da0054a74 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -145,6 +145,7 @@ struct btrfs_inode {
>   	 * different from prop_compress and takes precedence if set.
>   	 */
>   	u8 defrag_compress;
> +	s8 defrag_compress_level;
>
>   	/*
>   	 * Lock for counters and all fields used to determine if the inode is in
> diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
> index 968dae9539482..03a0287a78ea0 100644
> --- a/fs/btrfs/defrag.c
> +++ b/fs/btrfs/defrag.c
> @@ -1363,6 +1363,7 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,
>   	u64 last_byte;
>   	bool do_compress = (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS);
>   	int compress_type = BTRFS_COMPRESS_ZLIB;
> +	int compress_level = 0;
>   	int ret = 0;
>   	u32 extent_thresh = range->extent_thresh;
>   	pgoff_t start_index;
> @@ -1376,10 +1377,19 @@ 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;
> +			}

I am not familiar with the compress level, but
btrfs_compress_set_level() does extra clamping, maybe we also want to do
that too?

> +		} else {
> +			if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES)
> +				return -EINVAL;
> +			if (range->compress_type)
> +				compress_type = range->compress_type;
> +		}
>   	}
>
>   	if (extent_thresh == 0)
> @@ -1430,8 +1440,10 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,
>   			btrfs_inode_unlock(BTRFS_I(inode), 0);
>   			break;
>   		}
> -		if (do_compress)
> +		if (do_compress) {
>   			BTRFS_I(inode)->defrag_compress = compress_type;
> +			BTRFS_I(inode)->defrag_compress_level = compress_level;
> +		}
>   		ret = defrag_one_cluster(BTRFS_I(inode), ra, cur,
>   				cluster_end + 1 - cur, extent_thresh,
>   				newer_than, do_compress, &sectors_defragged,
> diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
> index be6d5a24bd4e6..2dae7ffd37133 100644
> --- a/fs/btrfs/fs.h
> +++ b/fs/btrfs/fs.h
> @@ -485,7 +485,7 @@ struct btrfs_fs_info {
>   	u64 last_trans_log_full_commit;
>   	unsigned long long mount_opt;
>
> -	unsigned long compress_type:4;
> +	int compress_type;
>   	int compress_level;
>   	u32 commit_interval;
>   	/*
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index fa04b027d53ac..156a9d4603391 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -925,6 +925,7 @@ static void compress_file_range(struct btrfs_work *work)
>   	unsigned int poff;
>   	int i;
>   	int compress_type = fs_info->compress_type;
> +	int compress_level= fs_info->compress_level;
>
>   	inode_should_defrag(inode, start, end, end - start + 1, SZ_16K);
>
> @@ -1007,13 +1008,15 @@ static void compress_file_range(struct btrfs_work *work)
>   		goto cleanup_and_bail_uncompressed;
>   	}
>
> -	if (inode->defrag_compress)
> +	if (inode->defrag_compress) {
>   		compress_type = inode->defrag_compress;
> -	else if (inode->prop_compress)
> +		compress_level= inode->defrag_compress_level;
> +	} else if (inode->prop_compress) {
>   		compress_type = inode->prop_compress;
> +	}
>
>   	/* Compression level is applied here. */
> -	ret = btrfs_compress_folios(compress_type, fs_info->compress_level,
> +	ret = btrfs_compress_folios(compress_type, compress_level,
>   				    mapping, start, folios, &nr_folios, &total_in,
>   				    &total_compressed);
>   	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;
> +	union {
> +		__u32 compress_type;
> +		struct {
> +			__u8 type;
> +			__s8 level;
> +		} compress;
> +	};
>
>   	/* spare for later */
>   	__u32 unused[4];

We have enough space left here, although u32 is overkilled for
compress_type, using the unused space for a new s8/s16/s32 member should
be fine.


Thanks,
Qu