---- 在 星期一, 2019-10-07 07:28:32 David Sterba <dste...@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 <cgxu...@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
> >
> >
> >
>
