On Tue, Jan 29, 2019 at 10:14:18AM +0200, Nikolay Borisov wrote:
>
>
> On 28.01.19 г. 23:24 ч., Dennis Zhou wrote:
> > Currently, the only user of set_level() is zlib which sets an internal
> > workspace parameter. As level is now plumbed into get_workspace(), this
> > can be handled there rather than separately.
> >
> > This repurposes set_level() to bound the level passed in so it can be
> > used when setting the mounts compression level and as well as verifying
> > the level before getting a workspace. The other benefit is this divides
> > the meaning of compress(0) and get_workspace(0). The former means we
> > want to use the default compression level of the compression type. The
> > latter means we can use any workspace available.
> >
> > Signed-off-by: Dennis Zhou <den...@kernel.org>
> > ---
> }
> > diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> > index e3627139bc5c..d607be40aa0e 100644
> > --- a/fs/btrfs/compression.h
> > +++ b/fs/btrfs/compression.h
> > @@ -90,7 +90,7 @@ blk_status_t btrfs_submit_compressed_write(struct inode
> > *inode, u64 start,
> > blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio
> > *bio,
> > int mirror_num, unsigned long bio_flags);
> >
> > -unsigned btrfs_compress_str2level(const char *str);
> > +unsigned int btrfs_compress_str2level(unsigned int type, const char *str);
> >
> > enum btrfs_compression_type {
> > BTRFS_COMPRESS_NONE = 0,
> > @@ -149,7 +149,7 @@ struct btrfs_compress_op {
> > unsigned long start_byte,
> > size_t srclen, size_t destlen);
> >
> > - void (*set_level)(struct list_head *ws, unsigned int type);
> > + unsigned int (*set_level)(unsigned int level);
>
> It might be good to document the return value since this is an
> interface. AFAICS implementations are required to return the actual
> level set irrespective of what level was passed in, no?
>
> > };
Yeah that makes sense. I've added a comment about it in
btrfs_compress_op.
> >
> > static void zlib_put_workspace(struct list_head *ws)
> > @@ -413,15 +418,14 @@ static int zlib_decompress(struct list_head *ws,
> > unsigned char *data_in,
> > return ret;
> > }
> >
> > -static void zlib_set_level(struct list_head *ws, unsigned int type)
> > +static unsigned int zlib_set_level(unsigned int level)
> > {
> > - struct workspace *workspace = list_entry(ws, struct workspace, list);
> > - unsigned int level = BTRFS_COMPRESS_LEVEL(type);
> > -
> > - if (level > 9)
> > + if (!level)
> > + level = BTRFS_ZLIB_DEFAULT_LEVEL;
> > + else if (level > 9)
> > level = 9;
>
> nit: This makes it a bit more obvious (IMO) that you are essentially
> doing max:
>
> if (!level)
> level = BTRFS_ZLIB_DEFAULT_LEVEL;
> level = max(level, 9);
>
I agree. I've updated it in both places. It should be min instead of max
though.
Thanks,
Dennis
