Re: [PATCH 2/3] btrfs: code cleanup for compression type

[Chengguang Xu <cgxu519@mykernel.net>]

 ---- 在 星期一, 2019-10-07 07:28:32 David Sterba <dsterba@suse.cz> 撰写 ----
 > On Sat, Oct 05, 2019 at 01:17:35PM +0800, Chengguang Xu wrote:
 > > Let BTRFS_COMPRESS_TYPES represents the total number
 > > of cmpressoin types and fix related calling places.
 > > It will be more safe when adding new compression type
 > > in the future.
 > 
 > I think we're not going to add a new type anytime soon, zstd provides
 > the choice between fast and good ratio. This itself is not an objection
 > to your patch but is not IMO the true reason for the changes.
 > 
 > Can you please describe the motivation behind the patches? Eg. if it's a
 > general cleanup or if there are other changes planned on top.

Actually, it's just a general cleanup. I found another enum in btrfs code for RAID types
and I think that usage makes me(at least :-)) easy to understand the code. So the
motivation is to keep code style consistency and  make the code a bit more readable.


 > 
 > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > > ---
 > >  fs/btrfs/compression.c  |  2 ++
 > >  fs/btrfs/compression.h  | 12 ++++++------
 > >  fs/btrfs/ioctl.c        |  2 +-
 > >  fs/btrfs/tree-checker.c |  4 ++--
 > >  4 files changed, 11 insertions(+), 9 deletions(-)
 > > 
 > > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
 > > index d70c46407420..93deaf0cc2b8 100644
 > > --- a/fs/btrfs/compression.c
 > > +++ b/fs/btrfs/compression.c
 > > @@ -39,6 +39,8 @@ const char* btrfs_compress_type2str(enum btrfs_compression_type type)
 > >      case BTRFS_COMPRESS_ZSTD:
 > >      case BTRFS_COMPRESS_NONE:
 > >          return btrfs_compress_types[type];
 > > +    default:
 > > +        break;
 > >      }
 > >  
 > >      return NULL;
 > > diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
 > > index dd392278ab3f..091ff3f986e5 100644
 > > --- a/fs/btrfs/compression.h
 > > +++ b/fs/btrfs/compression.h
 > > @@ -101,11 +101,11 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 > >  unsigned int btrfs_compress_str2level(unsigned int type, const char *str);
 > >  
 > >  enum btrfs_compression_type {
 > > -    BTRFS_COMPRESS_NONE  = 0,
 > > -    BTRFS_COMPRESS_ZLIB  = 1,
 > > -    BTRFS_COMPRESS_LZO   = 2,
 > > -    BTRFS_COMPRESS_ZSTD  = 3,
 > > -    BTRFS_COMPRESS_TYPES = 3,
 > > +    BTRFS_COMPRESS_NONE,
 > > +    BTRFS_COMPRESS_ZLIB,
 > > +    BTRFS_COMPRESS_LZO,
 > > +    BTRFS_COMPRESS_ZSTD,
 > > +    BTRFS_COMPRESS_TYPES
 > 
 > Please note that the on-disk format values should be expressed by the
 > values, even if it's the same as the automatic enum assignments.

I'll fix in v2.

 > 
 > Regarding change of the BTRFS_COMPRESS_TYPES value, I vaguely remember
 > we had patches for that but I don't recall why it was not changed. The
 > progs have an extra BTRFS_COMPRESS_LAST (== 4) that would be used the
 > same way as you do in this patch.

In previous patch, we had compression type(1-3, skip 0) in the code,
so there may be a bit of  confusion for BTRFS_COMPRESS_TYPES(==4) . 
I think it's not a problem now but maybe  change name to BTRFS_NR_COMPRESS_TYPES(like RAID type enum) 
is better.

 > 
 > BTRFS_COMPRESS_* is not in the public API so changing the value should
 > be safe, but needs double checking.
 > 
 > >  };
 > >  
 > >  struct workspace_manager {
 > > @@ -163,7 +163,7 @@ struct btrfs_compress_op {
 > >  };
 > >  
 > >  /* The heuristic workspaces are managed via the 0th workspace manager */
 > > -#define BTRFS_NR_WORKSPACE_MANAGERS    (BTRFS_COMPRESS_TYPES + 1)
 > > +#define BTRFS_NR_WORKSPACE_MANAGERS    BTRFS_COMPRESS_TYPES
 > >  
 > >  extern const struct btrfs_compress_op btrfs_heuristic_compress;
 > >  extern const struct btrfs_compress_op btrfs_zlib_compress;
 > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
 > > index de730e56d3f5..8c7196ed7ae0 100644
 > > --- a/fs/btrfs/ioctl.c
 > > +++ b/fs/btrfs/ioctl.c
 > > @@ -1411,7 +1411,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 > >          return -EINVAL;
 > >  
 > >      if (do_compress) {
 > > -        if (range->compress_type > BTRFS_COMPRESS_TYPES)
 > > +        if (range->compress_type >= BTRFS_COMPRESS_TYPES)
 > >              return -EINVAL;
 > >          if (range->compress_type)
 > >              compress_type = range->compress_type;
 > > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
 > > index f28f9725cef1..2d91c34bbf63 100644
 > > --- a/fs/btrfs/tree-checker.c
 > > +++ b/fs/btrfs/tree-checker.c
 > > @@ -168,11 +168,11 @@ static int check_extent_data_item(struct extent_buffer *leaf,
 > >       * Support for new compression/encryption must introduce incompat flag,
 > >       * and must be caught in open_ctree().
 > >       */
 > > -    if (btrfs_file_extent_compression(leaf, fi) > BTRFS_COMPRESS_TYPES) {
 > > +    if (btrfs_file_extent_compression(leaf, fi) >= BTRFS_COMPRESS_TYPES) {
 > >          file_extent_err(leaf, slot,
 > >      "invalid compression for file extent, have %u expect range [0, %u]",
 > >              btrfs_file_extent_compression(leaf, fi),
 > > -            BTRFS_COMPRESS_TYPES);
 > > +            BTRFS_COMPRESS_TYPES - 1);
 > >          return -EUCLEAN;
 > >      }
 > >      if (btrfs_file_extent_encryption(leaf, fi)) {
 > > -- 
 > > 2.21.0
 > > 
 > > 
 > > 
 >