linux-kernel - Re: [PATCH] btrfs/defrag: implement compression levels

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPjX3FfYmbdPLYTS5N5WUimqh=Dz41nVXbwm47iRW=Q3E7Wnhg@mail.gmail.com>
Date: Wed, 5 Mar 2025 08:08:35 +0100
From: Daniel Vacek <neelx@...e.com>
To: dsterba@...e.cz
Cc: Qu Wenruo <quwenruo.btrfs@....com>, Chris Mason <clm@...com>, 
	Josef Bacik <josef@...icpanda.com>, David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] btrfs/defrag: implement compression levels

On Wed, 5 Mar 2025 at 08:02, David Sterba <dsterba@...e.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@...e.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.