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;
+			}
+		} 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];
-- 
2.47.2

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

On Tue, 4 Mar 2025 at 22:31, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
> The feature itself looks good to me.
>
> Although not sure if a blank commit message is fine for this case.

Sigh, that's a mistake. I'll send a v2 with a few sentences.

> 在 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?

This is intentionally left to be limited later. There's no need to do
it at this point and the code is simpler. It's also compression
type/method agnostic.

> > +             } 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.

That is what I did originally, but discussing with Dave he suggested
this solution.

>
> Thanks,
> Qu

在 2025/3/5 17:32, Daniel Vacek 写道:
> On Tue, 4 Mar 2025 at 22:31, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
[...]
>>
>> I am not familiar with the compress level, but
>> btrfs_compress_set_level() does extra clamping, maybe we also want to do
>> that too?
> 
> This is intentionally left to be limited later. There's no need to do
> it at this point and the code is simpler. It's also compression
> type/method agnostic.

You're right, the level checks are done in the compression path already.

[...]
>>>        /* 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.
> 
> That is what I did originally, but discussing with Dave he suggested
> this solution.

Normally I would be fine with the union, to save some memory.

Maybe I'm a little paranoid, but the defrag ioctl flag check is only 
introduced last year by commit 173431b274a9 ("btrfs: defrag: reject 
unknown flags of btrfs_ioctl_defrag_range_args").

So it's possible that some older kernels don't have that commit, and may 
incorrectly continue by ignoring the flag.
Thankfully that should fail with -EINVAL (type always in the higher 
bits, thus always tricking the NR_COMPRESS_TYPES check.

If that layout (type in higher bits, level in lower bits) is 
intentionally, I'd say it's very clever.

Anyway either solution looks fine to me now.

With that commit message fixed:

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

Thanks,
Qu
> 
>>
>> Thanks,
>> Qu
> 

On Wed, Mar 05, 2025 at 06:14:16PM +1030, Qu Wenruo wrote:
> [...]
> >>>        /* 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.
> > 
> > That is what I did originally, but discussing with Dave he suggested
> > this solution.
> 
> Normally I would be fine with the union, to save some memory.
> 
> Maybe I'm a little paranoid, but the defrag ioctl flag check is only 
> introduced last year by commit 173431b274a9 ("btrfs: defrag: reject 
> unknown flags of btrfs_ioctl_defrag_range_args").

The commit has been backported to stable trees 4.19.307 5.10.210 5.15.149
5.4.269 6.1.76 6.6.15 6.7.3 , so we could assume the flags are
validated.

> So it's possible that some older kernels don't have that commit, and may 
> incorrectly continue by ignoring the flag.
> Thankfully that should fail with -EINVAL (type always in the higher 
> bits, thus always tricking the NR_COMPRESS_TYPES check.
> 
> If that layout (type in higher bits, level in lower bits) is 
> intentionally, I'd say it's very clever.
> 
> Anyway either solution looks fine to me now.

The layout also depends on endianness, but should not matter as long as
the flgags are validated. If not, either the level is ignored or it
fails due to the >= NR_COMPRESS_TYPES check. Both should be acceptable
as fallback.

在 2025/3/5 18:31, David Sterba 写道:
> On Wed, Mar 05, 2025 at 06:14:16PM +1030, Qu Wenruo wrote:
>> [...]
>>>>>         /* 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.
>>>
>>> That is what I did originally, but discussing with Dave he suggested
>>> this solution.
>>
>> Normally I would be fine with the union, to save some memory.
>>
>> Maybe I'm a little paranoid, but the defrag ioctl flag check is only
>> introduced last year by commit 173431b274a9 ("btrfs: defrag: reject
>> unknown flags of btrfs_ioctl_defrag_range_args").
>
> The commit has been backported to stable trees 4.19.307 5.10.210 5.15.149
> 5.4.269 6.1.76 6.6.15 6.7.3 , so we could assume the flags are
> validated.

I know it's backported to all stable kernels, but I still won't consider
a patch that's only one year old to be applied to all the kernels in the
wide.

Maybe I'm really paranoid on this.

>
>> So it's possible that some older kernels don't have that commit, and may
>> incorrectly continue by ignoring the flag.
>> Thankfully that should fail with -EINVAL (type always in the higher
>> bits, thus always tricking the NR_COMPRESS_TYPES check.
>>
>> If that layout (type in higher bits, level in lower bits) is
>> intentionally, I'd say it's very clever.
>>
>> Anyway either solution looks fine to me now.
>
> The layout also depends on endianness, but should not matter as long as
> the flgags are validated. If not, either the level is ignored or it
> fails due to the >= NR_COMPRESS_TYPES check. Both should be acceptable
> as fallback.

Since the fallback behavior is sane, I'm fine with the union solution.

Thanks,
Qu

On Wed, Mar 05, 2025 at 08:02:28AM +0100, Daniel Vacek wrote:
> > > @@ -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?
> 
> This is intentionally left to be limited later. There's no need to do
> it at this point and the code is simpler. It's also compression
> type/method agnostic.

This is input parameter validation so we should not postpone it until
the whole process starts. The complexity can be wrapped in helpers, we
already have that for various purposes like
compression_decompress_bio().

On Wed, 5 Mar 2025 at 08:05, David Sterba <dsterba@suse.cz> wrote:
>
> On Wed, Mar 05, 2025 at 08:02:28AM +0100, Daniel Vacek wrote:
> > > > @@ -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?
> >
> > This is intentionally left to be limited later. There's no need to do
> > it at this point and the code is simpler. It's also compression
> > type/method agnostic.
>
> This is input parameter validation so we should not postpone it until
> the whole process starts. The complexity can be wrapped in helpers, we
> already have that for various purposes like
> compression_decompress_bio().

OK, that makes sense. I'll add the check in v2.

Thaks guys.

On Wed, Mar 05, 2025 at 08:01:24AM +1030, Qu Wenruo wrote:
> 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?

Yes the level needs to be validated here as well.

> > @@ -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.

I suggested to do it like that, u32 is wasting space and the union trick
reusing existing space was already done e.g. in the balance filters.

On Wed, 5 Mar 2025 at 08:02, David Sterba <dsterba@suse.cz> wrote:
>
> On Wed, Mar 05, 2025 at 08:01:24AM +1030, Qu Wenruo wrote:
> > 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?
>
> Yes the level needs to be validated here as well.

The level is passed to btrfs_compress_folios() in
compress_file_range() and the first thing it does is precisely
btrfs_compress_set_level(). So I thought it's not needed to be done
twice.

> > > @@ -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.
>
> I suggested to do it like that, u32 is wasting space and the union trick
> reusing existing space was already done e.g. in the balance filters.

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@suse.com>
---
v3: Validate the level instead of clamping and fix the comment of the
    btrfs_ioctl_defrag_range_args structure.

v2: Fixed the commit message and added an explicit level range clamping.

 fs/btrfs/btrfs_inode.h     |  1 +
 fs/btrfs/compression.c     | 10 ++++++++++
 fs/btrfs/compression.h     |  1 +
 fs/btrfs/defrag.c          | 24 +++++++++++++++++++-----
 fs/btrfs/fs.h              |  2 +-
 fs/btrfs/inode.c           |  9 ++++++---
 include/uapi/linux/btrfs.h | 16 +++++++++++++---
 7 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index aa1f55cd81b79..238e4a08a52ae 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/compression.c b/fs/btrfs/compression.c
index 6d073e69af4e3..4191e9efc6951 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -980,6 +980,16 @@ static int btrfs_compress_set_level(unsigned int type, int level)
 	return level;
 }
 
+/*
+ * Check whether the @level is within the valid range for the given type.
+ */
+bool btrfs_compress_level_valid(unsigned int type, int level)
+{
+	const struct btrfs_compress_op *ops = btrfs_compress_op[type];
+
+	return ops->min_level <= level && level <= ops->max_level;
+}
+
 /* Wrapper around find_get_page(), with extra error message. */
 int btrfs_compress_filemap_get_folio(struct address_space *mapping, u64 start,
 				     struct folio **in_folio_ret)
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index 933178f03d8f8..df198623cc084 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -83,6 +83,7 @@ static inline u32 btrfs_calc_input_length(u64 range_end, u64 cur)
 int __init btrfs_init_compress(void);
 void __cold btrfs_exit_compress(void);
 
+bool btrfs_compress_level_valid(unsigned int type, int level);
 int btrfs_compress_folios(unsigned int type, int level, struct address_space *mapping,
 			  u64 start, struct folio **folios, unsigned long *out_folios,
 			 unsigned long *total_in, unsigned long *total_out);
diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
index 968dae9539482..513089b91b7b6 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,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;
+				if (!btrfs_compress_level_valid(compress_type, compress_level))
+					return -EINVAL;
+			}
+		} 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 +1442,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..dd27992ecb431 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..dd02160015b2b 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 {
@@ -640,10 +642,18 @@ struct btrfs_ioctl_defrag_range_args {
 
 	/*
 	 * which compression method to use if turning on compression
-	 * for this defrag operation.  If unspecified, zlib will
-	 * be used
+	 * for this defrag operation. If unspecified, zlib will be
+	 * used. If compression level is also being specified, set the
+	 * BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL flag and fill the compress
+	 * member structure instead of the compress_type field.
 	 */
-	__u32 compress_type;
+	union {
+		__u32 compress_type;
+		struct {
+			__u8 type;
+			__s8 level;
+		} compress;
+	};
 
 	/* spare for later */
 	__u32 unused[4];
-- 
2.47.2

在 2025/3/6 23:45, Daniel Vacek 写道:
> 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@suse.com>

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

Thanks,
Qu

> ---
> v3: Validate the level instead of clamping and fix the comment of the
>      btrfs_ioctl_defrag_range_args structure.
> 
> v2: Fixed the commit message and added an explicit level range clamping.
> 
>   fs/btrfs/btrfs_inode.h     |  1 +
>   fs/btrfs/compression.c     | 10 ++++++++++
>   fs/btrfs/compression.h     |  1 +
>   fs/btrfs/defrag.c          | 24 +++++++++++++++++++-----
>   fs/btrfs/fs.h              |  2 +-
>   fs/btrfs/inode.c           |  9 ++++++---
>   include/uapi/linux/btrfs.h | 16 +++++++++++++---
>   7 files changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index aa1f55cd81b79..238e4a08a52ae 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/compression.c b/fs/btrfs/compression.c
> index 6d073e69af4e3..4191e9efc6951 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -980,6 +980,16 @@ static int btrfs_compress_set_level(unsigned int type, int level)
>   	return level;
>   }
>   
> +/*
> + * Check whether the @level is within the valid range for the given type.
> + */
> +bool btrfs_compress_level_valid(unsigned int type, int level)
> +{
> +	const struct btrfs_compress_op *ops = btrfs_compress_op[type];
> +
> +	return ops->min_level <= level && level <= ops->max_level;
> +}
> +
>   /* Wrapper around find_get_page(), with extra error message. */
>   int btrfs_compress_filemap_get_folio(struct address_space *mapping, u64 start,
>   				     struct folio **in_folio_ret)
> diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> index 933178f03d8f8..df198623cc084 100644
> --- a/fs/btrfs/compression.h
> +++ b/fs/btrfs/compression.h
> @@ -83,6 +83,7 @@ static inline u32 btrfs_calc_input_length(u64 range_end, u64 cur)
>   int __init btrfs_init_compress(void);
>   void __cold btrfs_exit_compress(void);
>   
> +bool btrfs_compress_level_valid(unsigned int type, int level);
>   int btrfs_compress_folios(unsigned int type, int level, struct address_space *mapping,
>   			  u64 start, struct folio **folios, unsigned long *out_folios,
>   			 unsigned long *total_in, unsigned long *total_out);
> diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
> index 968dae9539482..513089b91b7b6 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,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;
> +				if (!btrfs_compress_level_valid(compress_type, compress_level))
> +					return -EINVAL;
> +			}
> +		} 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 +1442,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..dd27992ecb431 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..dd02160015b2b 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 {
> @@ -640,10 +642,18 @@ struct btrfs_ioctl_defrag_range_args {
>   
>   	/*
>   	 * which compression method to use if turning on compression
> -	 * for this defrag operation.  If unspecified, zlib will
> -	 * be used
> +	 * for this defrag operation. If unspecified, zlib will be
> +	 * used. If compression level is also being specified, set the
> +	 * BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL flag and fill the compress
> +	 * member structure instead of the compress_type field.
>   	 */
> -	__u32 compress_type;
> +	union {
> +		__u32 compress_type;
> +		struct {
> +			__u8 type;
> +			__s8 level;
> +		} compress;
> +	};
>   
>   	/* spare for later */
>   	__u32 unused[4];

On Thu, Mar 06, 2025 at 02:15:35PM +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@suse.com>
> ---
> v3: Validate the level instead of clamping and fix the comment of the
>     btrfs_ioctl_defrag_range_args structure.

Thanks, this looks good to me now, I'll add it to for-next.

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@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
---
v2: Fixed the commit message and added an explicit level range check.

 fs/btrfs/btrfs_inode.h     |  1 +
 fs/btrfs/compression.c     |  2 +-
 fs/btrfs/compression.h     |  1 +
 fs/btrfs/defrag.c          | 24 +++++++++++++++++++-----
 fs/btrfs/fs.h              |  2 +-
 fs/btrfs/inode.c           |  9 ++++++---
 include/uapi/linux/btrfs.h | 10 +++++++++-
 7 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index aa1f55cd81b79..238e4a08a52ae 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/compression.c b/fs/btrfs/compression.c
index 6d073e69af4e3..7106d19ee7bac 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -968,7 +968,7 @@ static void put_workspace(int type, struct list_head *ws)
  * Adjust @level according to the limits of the compression algorithm or
  * fallback to default
  */
-static int btrfs_compress_set_level(unsigned int type, int level)
+int btrfs_compress_set_level(unsigned int type, int level)
 {
 	const struct btrfs_compress_op *ops = btrfs_compress_op[type];
 
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index 933178f03d8f8..125509605f847 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -83,6 +83,7 @@ static inline u32 btrfs_calc_input_length(u64 range_end, u64 cur)
 int __init btrfs_init_compress(void);
 void __cold btrfs_exit_compress(void);
 
+int btrfs_compress_set_level(unsigned int type, int level);
 int btrfs_compress_folios(unsigned int type, int level, struct address_space *mapping,
 			  u64 start, struct folio **folios, unsigned long *out_folios,
 			 unsigned long *total_in, unsigned long *total_out);
diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
index 968dae9539482..3e9e2345a5683 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,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;
+				compress_level= btrfs_compress_set_level(compress_type,
+									 compress_level);
+			}
+		} 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 +1442,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..d26c005bf091a 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];
-- 
2.47.2

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@suse.com>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> ---
> 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
>

On Thu, 6 Mar 2025 at 09:27, David Sterba <dsterba@suse.cz> wrote:
>
> 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@suse.com>
> > Reviewed-by: Qu Wenruo <wqu@suse.com>
> > ---
> > 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.

I see. For some reason I still had the clamping on my mind. I'll send
a v3 with this.

> > @@ -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
> >