Re: [PATCH 03/11] btrfs: manage heuristic workspace as index 0

David Sterba
2019-01-31T16:10:59Z

On Mon, Jan 28, 2019 at 04:24:29PM -0500, Dennis Zhou wrote:
> While the heuristic workspaces aren't really compression workspaces,
> they use the same interface for managing them. So rather than branching,
> let's just handle them once again as the index 0 compression type.
>
> Signed-off-by: Dennis Zhou <dennis@kernel.org>
> +const struct btrfs_compress_op btrfs_heuristic_compress = {
> + .alloc_workspace = alloc_heuristic_ws,
> + .free_workspace = free_heuristic_ws,
> +};
> struct workspace_manager {
> struct list_head idle_ws;
> spinlock_t ws_lock;
> @@ -782,9 +789,8 @@ struct workspace_manager {
>
> static struct workspace_manager wsm[BTRFS_COMPRESS_TYPES];

This deserves a comment that the 0th workspace is for the heuristics.

> --- a/fs/btrfs/compression.h
> +++ b/fs/btrfs/compression.h
> @@ -97,7 +97,7 @@ enum btrfs_compression_type {
> BTRFS_COMPRESS_ZLIB = 1,
> BTRFS_COMPRESS_LZO = 2,
> BTRFS_COMPRESS_ZSTD = 3,
> - BTRFS_COMPRESS_TYPES = 3,
> + BTRFS_COMPRESS_TYPES = 4,

And here too, as there are only 3 compressors but 4 types as value of
the enum. Or rename BTRFS_COMPRESS_TYPES if you find a better name.

> };
>
> struct btrfs_compress_op {
> @@ -125,6 +125,7 @@ struct btrfs_compress_op {
> void (*set_level)(struct list_head *ws, unsigned int type);
> };
>
> +extern const struct btrfs_compress_op btrfs_heuristic_compress;
> extern const struct btrfs_compress_op btrfs_zlib_compress;
> extern const struct btrfs_compress_op btrfs_lzo_compress;
> extern const struct btrfs_compress_op btrfs_zstd_compress;
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 9c8e1734429c..20081465a451 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1410,7 +1410,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 a62e1e837a89..c88e146d8e99 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -133,9 +133,9 @@ static int check_extent_data_item(struct btrfs_fs_info *fs_info,
> * 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(fs_info, leaf, slot,
> - "invalid compression for file extent, have %u expect range [0, %u]",
> + "invalid compression for file extent, have %u expect range [0, %u)",
> btrfs_file_extent_compression(leaf, fi),
> BTRFS_COMPRESS_TYPES);

This might become a bit confusing, the message is updated to say [0, 4)
but I'm not sure this is commonly understood that 4 does not belong
there. Either do -1 or define a new enum that contains the maximum
number so BTRFS_COMPRESS_TYPES is not overloaded.