linux-kernel - Re: [PATCH v2] 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: <CAPjX3FeBVghdwQO8v+DGXzzFRcpfkHTprrpv=GtWbCPfyuZkmg@mail.gmail.com>
Date: Thu, 6 Mar 2025 14:15:10 +0100
From: Daniel Vacek <neelx@...e.com>
To: dsterba@...e.cz
Cc: Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>, David Sterba <dsterba@...e.com>, 
	Nick Terrell <terrelln@...com>, Qu Wenruo <wqu@...e.com>, linux-btrfs@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] btrfs/defrag: implement compression levels

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