diff mbox series

[v2] btrfs/defrag: implement compression levels

Message ID 20250305103235.719210-1-neelx@suse.com (mailing list archive)
State New, archived
Headers show
Series [v2] btrfs/defrag: implement compression levels | expand

Commit Message

Daniel Vacek March 5, 2025, 10:32 a.m. UTC 
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(-)

Comments

David Sterba March 6, 2025, 8:27 a.m. UTC | #1 
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
>
Daniel Vacek March 6, 2025, 1:15 p.m. UTC | #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
> >
diff mbox series

Patch

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];