diff mbox series

[02/15] btrfs: switch compression callbacks to direct calls

Message ID e8b9ace10f2df3e902158fa8f2ed2976993a759b.1571054758.git.dsterba@suse.com (mailing list archive)
State New, archived
Headers show
Series Remove callback indirections in compression code | expand

Commit Message

David Sterba Oct. 14, 2019, 12:22 p.m. UTC
The indirect calls bring some overhead due to spectre vulnerability
mitigations. The number of cases is small and below the threshold
(10-20) where indirect call would be better.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/compression.c | 77 +++++++++++++++++++++++++++++++++++++-----
 fs/btrfs/compression.h | 17 ----------
 fs/btrfs/lzo.c         |  3 --
 fs/btrfs/zlib.c        |  3 --
 fs/btrfs/zstd.c        |  3 --
 5 files changed, 69 insertions(+), 34 deletions(-)

Comments

Johannes Thumshirn Oct. 17, 2019, 9:42 a.m. UTC | #1
On 14/10/2019 14:22, David Sterba wrote:
> +static int compression_decompress_bio(int type, struct list_head *ws,
> +		struct compressed_bio *cb)
> +{
> +	switch (type) {
> +	case BTRFS_COMPRESS_ZLIB: return zlib_decompress_bio(ws, cb);
> +	case BTRFS_COMPRESS_LZO:  return lzo_decompress_bio(ws, cb);
> +	case BTRFS_COMPRESS_ZSTD: return zstd_decompress_bio(ws, cb);
> +	case BTRFS_COMPRESS_NONE:
> +	default:
> +		/*
> +		 * This can't happen, the type is validated several times
> +		 * before we get here.
> +		 */
> +		BUG();
> +	}
> +}
> +
> +static int compression_decompress(int type, struct list_head *ws,
> +               unsigned char *data_in, struct page *dest_page,
> +               unsigned long start_byte, size_t srclen, size_t destlen)
> +{
> +	switch (type) {
> +	case BTRFS_COMPRESS_ZLIB: return zlib_decompress(ws, data_in, dest_page,
> +						start_byte, srclen, destlen);
> +	case BTRFS_COMPRESS_LZO:  return lzo_decompress(ws, data_in, dest_page,
> +						start_byte, srclen, destlen);
> +	case BTRFS_COMPRESS_ZSTD: return zstd_decompress(ws, data_in, dest_page,
> +						start_byte, srclen, destlen);
> +	case BTRFS_COMPRESS_NONE:
> +	default:
> +		/*
> +		 * This can't happen, the type is validated several times
> +		 * before we get here.
> +		 */
> +		BUG();
> +	}
> +}

Hmm should we really BUG() here? Maybe throw in an ASSERT(), so we can
catch it in debug builds but not halt the machine (even if it
theoretically can't happen).
David Sterba Oct. 17, 2019, 11:24 a.m. UTC | #2
On Thu, Oct 17, 2019 at 11:42:53AM +0200, Johannes Thumshirn wrote:
> On 14/10/2019 14:22, David Sterba wrote:
> > +static int compression_decompress_bio(int type, struct list_head *ws,
> > +		struct compressed_bio *cb)
> > +{
> > +	switch (type) {
> > +	case BTRFS_COMPRESS_ZLIB: return zlib_decompress_bio(ws, cb);
> > +	case BTRFS_COMPRESS_LZO:  return lzo_decompress_bio(ws, cb);
> > +	case BTRFS_COMPRESS_ZSTD: return zstd_decompress_bio(ws, cb);
> > +	case BTRFS_COMPRESS_NONE:
> > +	default:
> > +		/*
> > +		 * This can't happen, the type is validated several times
> > +		 * before we get here.
> > +		 */
> > +		BUG();
> > +	}
> > +}
> > +
> > +static int compression_decompress(int type, struct list_head *ws,
> > +               unsigned char *data_in, struct page *dest_page,
> > +               unsigned long start_byte, size_t srclen, size_t destlen)
> > +{
> > +	switch (type) {
> > +	case BTRFS_COMPRESS_ZLIB: return zlib_decompress(ws, data_in, dest_page,
> > +						start_byte, srclen, destlen);
> > +	case BTRFS_COMPRESS_LZO:  return lzo_decompress(ws, data_in, dest_page,
> > +						start_byte, srclen, destlen);
> > +	case BTRFS_COMPRESS_ZSTD: return zstd_decompress(ws, data_in, dest_page,
> > +						start_byte, srclen, destlen);
> > +	case BTRFS_COMPRESS_NONE:
> > +	default:
> > +		/*
> > +		 * This can't happen, the type is validated several times
> > +		 * before we get here.
> > +		 */
> > +		BUG();
> > +	}
> > +}
> 
> Hmm should we really BUG() here? Maybe throw in an ASSERT(), so we can
> catch it in debug builds but not halt the machine (even if it
> theoretically can't happen).

Debug builds would catch it for sure, regardless of bug or assert, but
assert would not be on builds without CONFIG_BTRFS_ASSERT and continuing
here is not correct. If the code reatches the switch with a wrong value,
then the wrong value has been used to dereference an array in the
caller, so the state of the system is not defined. I don't see a better
option than BUG, thouth we don't want to add new ones. At least with the
comment it should be clear that it's not somebody's laziness to do
proper error handling, here it's not possible to do it at all.
diff mbox series

Patch

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 8611a8b3321a..87bac8f73a99 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -86,6 +86,70 @@  bool btrfs_compress_is_valid_type(const char *str, size_t len)
 	return false;
 }
 
+static int compression_compress_pages(int type, struct list_head *ws,
+               struct address_space *mapping, u64 start, struct page **pages,
+               unsigned long *out_pages, unsigned long *total_in,
+               unsigned long *total_out)
+{
+	switch (type) {
+	case BTRFS_COMPRESS_ZLIB:
+		return zlib_compress_pages(ws, mapping, start, pages,
+				out_pages, total_in, total_out);
+	case BTRFS_COMPRESS_LZO:
+		return lzo_compress_pages(ws, mapping, start, pages,
+				out_pages, total_in, total_out);
+	case BTRFS_COMPRESS_ZSTD:
+		return zstd_compress_pages(ws, mapping, start, pages,
+				out_pages, total_in, total_out);
+	case BTRFS_COMPRESS_NONE:
+	default:
+		/*
+		 * This can't happen, the type is validated several times
+		 * before we get here. As a sane fallback, return what the
+		 * callers will understand as 'no compression happened'.
+		 */
+		return -E2BIG;
+	}
+}
+
+static int compression_decompress_bio(int type, struct list_head *ws,
+		struct compressed_bio *cb)
+{
+	switch (type) {
+	case BTRFS_COMPRESS_ZLIB: return zlib_decompress_bio(ws, cb);
+	case BTRFS_COMPRESS_LZO:  return lzo_decompress_bio(ws, cb);
+	case BTRFS_COMPRESS_ZSTD: return zstd_decompress_bio(ws, cb);
+	case BTRFS_COMPRESS_NONE:
+	default:
+		/*
+		 * This can't happen, the type is validated several times
+		 * before we get here.
+		 */
+		BUG();
+	}
+}
+
+static int compression_decompress(int type, struct list_head *ws,
+               unsigned char *data_in, struct page *dest_page,
+               unsigned long start_byte, size_t srclen, size_t destlen)
+{
+	switch (type) {
+	case BTRFS_COMPRESS_ZLIB: return zlib_decompress(ws, data_in, dest_page,
+						start_byte, srclen, destlen);
+	case BTRFS_COMPRESS_LZO:  return lzo_decompress(ws, data_in, dest_page,
+						start_byte, srclen, destlen);
+	case BTRFS_COMPRESS_ZSTD: return zstd_decompress(ws, data_in, dest_page,
+						start_byte, srclen, destlen);
+	case BTRFS_COMPRESS_NONE:
+	default:
+		/*
+		 * This can't happen, the type is validated several times
+		 * before we get here.
+		 */
+		BUG();
+	}
+}
+
 static int btrfs_decompress_bio(struct compressed_bio *cb);
 
 static inline int compressed_bio_size(struct btrfs_fs_info *fs_info,
@@ -1074,10 +1138,8 @@  int btrfs_compress_pages(unsigned int type_level, struct address_space *mapping,
 
 	level = btrfs_compress_set_level(type, level);
 	workspace = get_workspace(type, level);
-	ret = btrfs_compress_op[type]->compress_pages(workspace, mapping,
-						      start, pages,
-						      out_pages,
-						      total_in, total_out);
+	ret = compression_compress_pages(type, workspace, mapping, start, pages,
+					 out_pages, total_in, total_out);
 	put_workspace(type, workspace);
 	return ret;
 }
@@ -1103,7 +1165,7 @@  static int btrfs_decompress_bio(struct compressed_bio *cb)
 	int type = cb->compress_type;
 
 	workspace = get_workspace(type, 0);
-	ret = btrfs_compress_op[type]->decompress_bio(workspace, cb);
+	ret = compression_decompress_bio(type, workspace, cb);
 	put_workspace(type, workspace);
 
 	return ret;
@@ -1121,9 +1183,8 @@  int btrfs_decompress(int type, unsigned char *data_in, struct page *dest_page,
 	int ret;
 
 	workspace = get_workspace(type, 0);
-	ret = btrfs_compress_op[type]->decompress(workspace, data_in,
-						  dest_page, start_byte,
-						  srclen, destlen);
+	ret = compression_decompress(type, workspace, data_in, dest_page,
+				     start_byte, srclen, destlen);
 	put_workspace(type, workspace);
 
 	return ret;
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index 52dce1176f89..7db14d3166b5 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -140,23 +140,6 @@  struct btrfs_compress_op {
 
 	void (*free_workspace)(struct list_head *workspace);
 
-	int (*compress_pages)(struct list_head *workspace,
-			      struct address_space *mapping,
-			      u64 start,
-			      struct page **pages,
-			      unsigned long *out_pages,
-			      unsigned long *total_in,
-			      unsigned long *total_out);
-
-	int (*decompress_bio)(struct list_head *workspace,
-				struct compressed_bio *cb);
-
-	int (*decompress)(struct list_head *workspace,
-			  unsigned char *data_in,
-			  struct page *dest_page,
-			  unsigned long start_byte,
-			  size_t srclen, size_t destlen);
-
 	/* Maximum level supported by the compression algorithm */
 	unsigned int max_level;
 	unsigned int default_level;
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index 04a6815ea9cb..9417944ec829 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -509,9 +509,6 @@  const struct btrfs_compress_op btrfs_lzo_compress = {
 	.put_workspace		= lzo_put_workspace,
 	.alloc_workspace	= lzo_alloc_workspace,
 	.free_workspace		= lzo_free_workspace,
-	.compress_pages		= lzo_compress_pages,
-	.decompress_bio		= lzo_decompress_bio,
-	.decompress		= lzo_decompress,
 	.max_level		= 1,
 	.default_level		= 1,
 };
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index 4091f94ba378..8bb6f19ab369 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -420,9 +420,6 @@  const struct btrfs_compress_op btrfs_zlib_compress = {
 	.put_workspace		= zlib_put_workspace,
 	.alloc_workspace	= zlib_alloc_workspace,
 	.free_workspace		= zlib_free_workspace,
-	.compress_pages		= zlib_compress_pages,
-	.decompress_bio		= zlib_decompress_bio,
-	.decompress		= zlib_decompress,
 	.max_level		= 9,
 	.default_level		= BTRFS_ZLIB_DEFAULT_LEVEL,
 };
diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
index b3728220c329..5f17c741d167 100644
--- a/fs/btrfs/zstd.c
+++ b/fs/btrfs/zstd.c
@@ -713,9 +713,6 @@  const struct btrfs_compress_op btrfs_zstd_compress = {
 	.put_workspace = zstd_put_workspace,
 	.alloc_workspace = zstd_alloc_workspace,
 	.free_workspace = zstd_free_workspace,
-	.compress_pages = zstd_compress_pages,
-	.decompress_bio = zstd_decompress_bio,
-	.decompress = zstd_decompress,
 	.max_level	= ZSTD_BTRFS_MAX_LEVEL,
 	.default_level	= ZSTD_BTRFS_DEFAULT_LEVEL,
 };