| Message ID | 20190204202008.51652-10-dennis@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add zstd compression level support | expand |
On Mon, Feb 04, 2019 at 03:20:05PM -0500, Dennis Zhou wrote: > -unsigned int btrfs_compress_str2level(const char *str) > +unsigned int btrfs_compress_str2level(unsigned int type, const char *str) > { > - if (strncmp(str, "zlib", 4) != 0) > + unsigned int level; > + int ret; > + > + if (!type) > return 0; > > - /* Accepted form: zlib:1 up to zlib:9 and nothing left after the number */ > - if (str[4] == ':' && '1' <= str[5] && str[5] <= '9' && str[6] == 0) > - return str[5] - '0'; > + if (str[0] == ':') { > + ret = kstrtouint(str + 1, 10, &level); The docs kstrtouint of say that initial + is also accepted, I'd rather keep the level specification strict, ie. no "zlib:+3" and no garbage after the number. The validation is currently missing but I think we should catch levels out of range during mount/remount. The fallback to default is a safety but wrong specification should be communicated to the user early.
On Tue, Feb 05, 2019 at 08:06:37PM +0100, David Sterba wrote: > On Mon, Feb 04, 2019 at 03:20:05PM -0500, Dennis Zhou wrote: > > -unsigned int btrfs_compress_str2level(const char *str) > > +unsigned int btrfs_compress_str2level(unsigned int type, const char *str) > > { > > - if (strncmp(str, "zlib", 4) != 0) > > + unsigned int level; > > + int ret; > > + > > + if (!type) > > return 0; > > > > - /* Accepted form: zlib:1 up to zlib:9 and nothing left after the number */ > > - if (str[4] == ':' && '1' <= str[5] && str[5] <= '9' && str[6] == 0) > > - return str[5] - '0'; > > + if (str[0] == ':') { > > + ret = kstrtouint(str + 1, 10, &level); > > The docs kstrtouint of say that initial + is also accepted, I'd rather > keep the level specification strict, ie. no "zlib:+3" and no garbage > after the number. > > The validation is currently missing but I think we should catch levels > out of range during mount/remount. The fallback to default is a safety > but wrong specification should be communicated to the user early. Ok. To make sure I understand properly for improper level (ie "30", "+3", "+3d") set the level to default (already done) and pr_warn saying invalid level? Thanks, Dennis
On Tue, Feb 05, 2019 at 02:32:54PM -0500, Dennis Zhou wrote: > On Tue, Feb 05, 2019 at 08:06:37PM +0100, David Sterba wrote: > > On Mon, Feb 04, 2019 at 03:20:05PM -0500, Dennis Zhou wrote: > > > -unsigned int btrfs_compress_str2level(const char *str) > > > +unsigned int btrfs_compress_str2level(unsigned int type, const char *str) > > > { > > > - if (strncmp(str, "zlib", 4) != 0) > > > + unsigned int level; > > > + int ret; > > > + > > > + if (!type) > > > return 0; > > > > > > - /* Accepted form: zlib:1 up to zlib:9 and nothing left after the number */ > > > - if (str[4] == ':' && '1' <= str[5] && str[5] <= '9' && str[6] == 0) > > > - return str[5] - '0'; > > > + if (str[0] == ':') { > > > + ret = kstrtouint(str + 1, 10, &level); > > > > The docs kstrtouint of say that initial + is also accepted, I'd rather > > keep the level specification strict, ie. no "zlib:+3" and no garbage > > after the number. > > > > The validation is currently missing but I think we should catch levels > > out of range during mount/remount. The fallback to default is a safety > > but wrong specification should be communicated to the user early. > > Ok. To make sure I understand properly for improper level (ie "30", > "+3", "+3d") set the level to default (already done) and pr_warn saying > invalid level? So we have (at least) two ways how to handle: - warn and fail the mount -- catch typos and the like, continuing with default could cause problems later though not that severe as the compressionw would be different than expected, but this could be considered a usability bug - only warn and continue with the default -- not mounting a root filesystem just because of a typo can be worse than mounting with wrong level; as long as there's a warning in the log, the user still has a working system and can fix it manually Both have some pros and cons and initially I was more inclined to pick the 1st option, but now that I'm thinking about that again, the other has some merit. Given that zlib now falls back to default for unrecognized level, we should probably stick to that for zstd too.
On Tue, Feb 05, 2019 at 08:54:15PM +0100, David Sterba wrote: > On Tue, Feb 05, 2019 at 02:32:54PM -0500, Dennis Zhou wrote: > > On Tue, Feb 05, 2019 at 08:06:37PM +0100, David Sterba wrote: > > > On Mon, Feb 04, 2019 at 03:20:05PM -0500, Dennis Zhou wrote: > > > > -unsigned int btrfs_compress_str2level(const char *str) > > > > +unsigned int btrfs_compress_str2level(unsigned int type, const char *str) > > > > { > > > > - if (strncmp(str, "zlib", 4) != 0) > > > > + unsigned int level; > > > > + int ret; > > > > + > > > > + if (!type) > > > > return 0; > > > > > > > > - /* Accepted form: zlib:1 up to zlib:9 and nothing left after the number */ > > > > - if (str[4] == ':' && '1' <= str[5] && str[5] <= '9' && str[6] == 0) > > > > - return str[5] - '0'; > > > > + if (str[0] == ':') { > > > > + ret = kstrtouint(str + 1, 10, &level); > > > > > > The docs kstrtouint of say that initial + is also accepted, I'd rather > > > keep the level specification strict, ie. no "zlib:+3" and no garbage > > > after the number. > > > > > > The validation is currently missing but I think we should catch levels > > > out of range during mount/remount. The fallback to default is a safety > > > but wrong specification should be communicated to the user early. > > > > Ok. To make sure I understand properly for improper level (ie "30", > > "+3", "+3d") set the level to default (already done) and pr_warn saying > > invalid level? > > So we have (at least) two ways how to handle: > > - warn and fail the mount -- catch typos and the like, continuing with > default could cause problems later though not that severe as the > compressionw would be different than expected, but this could be > considered a usability bug > > - only warn and continue with the default -- not mounting a root > filesystem just because of a typo can be worse than mounting with > wrong level; as long as there's a warning in the log, the user still > has a working system and can fix it manually > > Both have some pros and cons and initially I was more inclined to pick > the 1st option, but now that I'm thinking about that again, the other > has some merit. > > Given that zlib now falls back to default for unrecognized level, we > should probably stick to that for zstd too. Ok. With that I'll run a v3 adding a warning for a '+' or invalid input. I think moving compression to being a property rather than a mount option would be ideal. That would enable multiple compression levels per mount and prevent remounting with incorrect mount options. Thanks, Dennis
From ef64a8d1f4ec44f52bd13a825288a91667121708 Mon Sep 17 00:00:00 2001 From: Dennis Zhou <dennis@kernel.org> Date: Thu, 17 Jan 2019 12:13:27 -0800 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 <dennis@kernel.org> --- v2: - warn on bad level format eg: zlib:c100 - warn on level out of bounds eg: zlib:10 fs/btrfs/compression.c | 27 +++++++++++++++++++-------- fs/btrfs/compression.h | 10 ++++++++-- fs/btrfs/lzo.c | 3 ++- fs/btrfs/super.c | 4 +++- fs/btrfs/zlib.c | 21 ++++++++++++++------- fs/btrfs/zstd.c | 3 ++- 6 files changed, 48 insertions(+), 20 deletions(-) diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index a54b210739bc..560a926c2e2f 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -1007,8 +1007,6 @@ int btrfs_compress_pages(unsigned int type_level, struct address_space *mapping, int ret; workspace = get_workspace(type, level); - - btrfs_compress_op[type]->set_level(workspace, level); ret = btrfs_compress_op[type]->compress_pages(workspace, mapping, start, pages, out_pages, @@ -1561,14 +1559,27 @@ int btrfs_compress_heuristic(struct inode *inode, u64 start, u64 end) return ret; } -unsigned int btrfs_compress_str2level(const char *str) +unsigned int btrfs_compress_str2level(unsigned int type, const char *str) { - if (strncmp(str, "zlib", 4) != 0) + unsigned int level = 0; + int ret; + + if (!type) return 0; - /* Accepted form: zlib:1 up to zlib:9 and nothing left after the number */ - if (str[4] == ':' && '1' <= str[5] && str[5] <= '9' && str[6] == 0) - return str[5] - '0'; + if (str[0] == ':') { + if (str[1] != '+') { + ret = kstrtouint(str + 1, 10, &level); + if (!ret) + goto set_level; + } + + pr_warn("BTRFS: invalid compression level %s, using default", + str + 1); + } + +set_level: + level = btrfs_compress_op[type]->set_level(level); - return BTRFS_ZLIB_DEFAULT_LEVEL; + return level; } diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h index 32ba40ebae1d..3f2d36b0aabe 100644 --- a/fs/btrfs/compression.h +++ b/fs/btrfs/compression.h @@ -97,7 +97,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, @@ -156,7 +156,13 @@ struct btrfs_compress_op { unsigned long start_byte, size_t srclen, size_t destlen); - void (*set_level)(struct list_head *ws, unsigned int type); + /* + * This bounds the level set by the user to be within range of + * a particular compression type. It returns the level that + * will be used if the level is out of bounds or the default + * if 0 is passed in. + */ + unsigned int (*set_level)(unsigned int level); }; /* the heuristic workspaces are managed via the 0th workspace manager */ diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c index f132af45a924..579d53ae256f 100644 --- a/fs/btrfs/lzo.c +++ b/fs/btrfs/lzo.c @@ -507,8 +507,9 @@ static int lzo_decompress(struct list_head *ws, unsigned char *data_in, return ret; } -static void lzo_set_level(struct list_head *ws, unsigned int type) +static unsigned int lzo_set_level(unsigned int level) { + return 0; } const struct btrfs_compress_op btrfs_lzo_compress = { diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index c5586ffd1426..b28dff207383 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -529,7 +529,9 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, if (token != Opt_compress && token != Opt_compress_force) info->compress_level = - btrfs_compress_str2level(args[0].from); + btrfs_compress_str2level( + BTRFS_COMPRESS_ZLIB, + args[0].from + 4); btrfs_set_opt(info->mount_opt, COMPRESS); btrfs_clear_opt(info->mount_opt, NODATACOW); btrfs_clear_opt(info->mount_opt, NODATASUM); diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c index c0ef09bef3b3..439dad221e9a 100644 --- a/fs/btrfs/zlib.c +++ b/fs/btrfs/zlib.c @@ -41,7 +41,12 @@ static void zlib_cleanup_workspace_manager(void) static struct list_head *zlib_get_workspace(unsigned int level) { - return btrfs_get_workspace(&wsm, level); + struct list_head *ws = btrfs_get_workspace(&wsm, level); + struct workspace *workspace = list_entry(ws, struct workspace, list); + + workspace->level = level; + + return ws; } static void zlib_put_workspace(struct list_head *ws) @@ -413,15 +418,17 @@ 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) { + return BTRFS_ZLIB_DEFAULT_LEVEL; + } else if (level > 9) { level = 9; + pr_warn("BTRFS: zlib level > max level, using max level: %u", + level); + } - workspace->level = level > 0 ? level : 3; + return level; } const struct btrfs_compress_op btrfs_zlib_compress = { diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c index 404101864220..43f3be755b8c 100644 --- a/fs/btrfs/zstd.c +++ b/fs/btrfs/zstd.c @@ -441,8 +441,9 @@ static int zstd_decompress(struct list_head *ws, unsigned char *data_in, return ret; } -static void zstd_set_level(struct list_head *ws, unsigned int type) +static unsigned int zstd_set_level(unsigned int level) { + return ZSTD_BTRFS_DEFAULT_LEVEL; } const struct btrfs_compress_op btrfs_zstd_compress = {
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index a54b210739bc..dc4afd7d9027 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -1007,8 +1007,6 @@ int btrfs_compress_pages(unsigned int type_level, struct address_space *mapping, int ret; workspace = get_workspace(type, level); - - btrfs_compress_op[type]->set_level(workspace, level); ret = btrfs_compress_op[type]->compress_pages(workspace, mapping, start, pages, out_pages, @@ -1561,14 +1559,21 @@ int btrfs_compress_heuristic(struct inode *inode, u64 start, u64 end) return ret; } -unsigned int btrfs_compress_str2level(const char *str) +unsigned int btrfs_compress_str2level(unsigned int type, const char *str) { - if (strncmp(str, "zlib", 4) != 0) + unsigned int level; + int ret; + + if (!type) return 0; - /* Accepted form: zlib:1 up to zlib:9 and nothing left after the number */ - if (str[4] == ':' && '1' <= str[5] && str[5] <= '9' && str[6] == 0) - return str[5] - '0'; + if (str[0] == ':') { + ret = kstrtouint(str + 1, 10, &level); + if (ret) + level = 0; + } + + level = btrfs_compress_op[type]->set_level(level); - return BTRFS_ZLIB_DEFAULT_LEVEL; + return level; } diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h index 32ba40ebae1d..3f2d36b0aabe 100644 --- a/fs/btrfs/compression.h +++ b/fs/btrfs/compression.h @@ -97,7 +97,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, @@ -156,7 +156,13 @@ struct btrfs_compress_op { unsigned long start_byte, size_t srclen, size_t destlen); - void (*set_level)(struct list_head *ws, unsigned int type); + /* + * This bounds the level set by the user to be within range of + * a particular compression type. It returns the level that + * will be used if the level is out of bounds or the default + * if 0 is passed in. + */ + unsigned int (*set_level)(unsigned int level); }; /* the heuristic workspaces are managed via the 0th workspace manager */ diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c index f132af45a924..579d53ae256f 100644 --- a/fs/btrfs/lzo.c +++ b/fs/btrfs/lzo.c @@ -507,8 +507,9 @@ static int lzo_decompress(struct list_head *ws, unsigned char *data_in, return ret; } -static void lzo_set_level(struct list_head *ws, unsigned int type) +static unsigned int lzo_set_level(unsigned int level) { + return 0; } const struct btrfs_compress_op btrfs_lzo_compress = { diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index c5586ffd1426..b28dff207383 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -529,7 +529,9 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, if (token != Opt_compress && token != Opt_compress_force) info->compress_level = - btrfs_compress_str2level(args[0].from); + btrfs_compress_str2level( + BTRFS_COMPRESS_ZLIB, + args[0].from + 4); btrfs_set_opt(info->mount_opt, COMPRESS); btrfs_clear_opt(info->mount_opt, NODATACOW); btrfs_clear_opt(info->mount_opt, NODATASUM); diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c index c0ef09bef3b3..77363f663868 100644 --- a/fs/btrfs/zlib.c +++ b/fs/btrfs/zlib.c @@ -41,7 +41,12 @@ static void zlib_cleanup_workspace_manager(void) static struct list_head *zlib_get_workspace(unsigned int level) { - return btrfs_get_workspace(&wsm, level); + struct list_head *ws = btrfs_get_workspace(&wsm, level); + struct workspace *workspace = list_entry(ws, struct workspace, list); + + workspace->level = level; + + return ws; } static void zlib_put_workspace(struct list_head *ws) @@ -413,15 +418,12 @@ 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) - level = 9; + if (!level) + return BTRFS_ZLIB_DEFAULT_LEVEL; - workspace->level = level > 0 ? level : 3; + return min_t(unsigned int, level, 9); } const struct btrfs_compress_op btrfs_zlib_compress = { diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c index 404101864220..43f3be755b8c 100644 --- a/fs/btrfs/zstd.c +++ b/fs/btrfs/zstd.c @@ -441,8 +441,9 @@ static int zstd_decompress(struct list_head *ws, unsigned char *data_in, return ret; } -static void zstd_set_level(struct list_head *ws, unsigned int type) +static unsigned int zstd_set_level(unsigned int level) { + return ZSTD_BTRFS_DEFAULT_LEVEL; } const struct btrfs_compress_op btrfs_zstd_compress = {
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 <dennis@kernel.org> --- fs/btrfs/compression.c | 21 +++++++++++++-------- fs/btrfs/compression.h | 10 ++++++++-- fs/btrfs/lzo.c | 3 ++- fs/btrfs/super.c | 4 +++- fs/btrfs/zlib.c | 18 ++++++++++-------- fs/btrfs/zstd.c | 3 ++- 6 files changed, 38 insertions(+), 21 deletions(-)