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

[David Sterba <dsterba@suse.cz>]

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.