[PATCH v3 1/2] squashfs: add the mount parameter theads=<single|multi|percpu>

Xiaoming Ni posted 2 patches 3 years, 7 months ago
There is a newer version of this series
[PATCH v3 1/2] squashfs: add the mount parameter theads=<single|multi|percpu>
Posted by Xiaoming Ni 3 years, 7 months ago
Squashfs supports three decompression concurrency modes:
	Single-thread mode: concurrent reads are blocked and the memory overhead
is small.
	Multi-thread mode/percpu mode: reduces concurrent read blocking but
increases memory overhead.

The corresponding schema must be fixed at compile time. During mounting,
the concurrent decompression mode cannot be adjusted based on file read
blocking.

The mount parameter theads=<single|multi|percpu> is added to select
the concurrent decompression mode of a single SquashFS file system
image.

Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
 fs/squashfs/Kconfig                     | 40 ++++++++++++++++++++++----
 fs/squashfs/decompressor_multi.c        | 28 ++++++++++--------
 fs/squashfs/decompressor_multi_percpu.c | 39 +++++++++++++++----------
 fs/squashfs/decompressor_single.c       | 23 +++++++++------
 fs/squashfs/squashfs.h                  | 43 ++++++++++++++++++++++++----
 fs/squashfs/squashfs_fs_sb.h            |  3 +-
 fs/squashfs/super.c                     | 50 ++++++++++++++++++++++++++++++++-
 7 files changed, 180 insertions(+), 46 deletions(-)

diff --git a/fs/squashfs/Kconfig b/fs/squashfs/Kconfig
index 916e78fabcaa..9c2827459f40 100644
--- a/fs/squashfs/Kconfig
+++ b/fs/squashfs/Kconfig
@@ -54,9 +54,37 @@ config SQUASHFS_FILE_DIRECT
 
 endchoice
 
+config SQUASHFS_DECOMP_SINGLE
+	depends on SQUASHFS
+	def_bool n
+
+config SQUASHFS_DECOMP_MULTI
+	depends on SQUASHFS
+	def_bool n
+
+config SQUASHFS_DECOMP_MULTI_PERCPU
+	depends on SQUASHFS
+	def_bool n
+
+config SQUASHFS_CHOICE_DECOMP_BY_MOUNT
+	bool "Select the parallel decompression mode during mount"
+	depends on SQUASHFS
+	default n
+	select SQUASHFS_DECOMP_SINGLE
+	select SQUASHFS_DECOMP_MULTI
+	select SQUASHFS_DECOMP_MULTI_PERCPU
+	help
+	  Compile all parallel decompression modes and specify the
+	  decompression mode by setting "threads=" during mount.
+
+	  	  threads=<single|multi|percpu>
+
+	  default Decompressor parallelisation is SQUASHFS_DECOMP_SINGLE
+
 choice
-	prompt "Decompressor parallelisation options"
+	prompt "Select decompression parallel mode at compile time"
 	depends on SQUASHFS
+	depends on !SQUASHFS_CHOICE_DECOMP_BY_MOUNT
 	help
 	  Squashfs now supports three parallelisation options for
 	  decompression.  Each one exhibits various trade-offs between
@@ -64,15 +92,17 @@ choice
 
 	  If in doubt, select "Single threaded compression"
 
-config SQUASHFS_DECOMP_SINGLE
+config SQUASHFS_COMPILE_DECOMP_SINGLE
 	bool "Single threaded compression"
+	select SQUASHFS_DECOMP_SINGLE
 	help
 	  Traditionally Squashfs has used single-threaded decompression.
 	  Only one block (data or metadata) can be decompressed at any
 	  one time.  This limits CPU and memory usage to a minimum.
 
-config SQUASHFS_DECOMP_MULTI
+config SQUASHFS_COMPILE_DECOMP_MULTI
 	bool "Use multiple decompressors for parallel I/O"
+	select SQUASHFS_DECOMP_MULTI
 	help
 	  By default Squashfs uses a single decompressor but it gives
 	  poor performance on parallel I/O workloads when using multiple CPU
@@ -85,8 +115,9 @@ config SQUASHFS_DECOMP_MULTI
 	  decompressors per core.  It dynamically allocates decompressors
 	  on a demand basis.
 
-config SQUASHFS_DECOMP_MULTI_PERCPU
+config SQUASHFS_COMPILE_DECOMP_MULTI_PERCPU
 	bool "Use percpu multiple decompressors for parallel I/O"
+	select SQUASHFS_DECOMP_MULTI_PERCPU
 	help
 	  By default Squashfs uses a single decompressor but it gives
 	  poor performance on parallel I/O workloads when using multiple CPU
@@ -95,7 +126,6 @@ config SQUASHFS_DECOMP_MULTI_PERCPU
 	  This decompressor implementation uses a maximum of one
 	  decompressor per core.  It uses percpu variables to ensure
 	  decompression is load-balanced across the cores.
-
 endchoice
 
 config SQUASHFS_XATTR
diff --git a/fs/squashfs/decompressor_multi.c b/fs/squashfs/decompressor_multi.c
index db9f12a3ea05..7b2723b77e75 100644
--- a/fs/squashfs/decompressor_multi.c
+++ b/fs/squashfs/decompressor_multi.c
@@ -29,13 +29,12 @@
 #define MAX_DECOMPRESSOR	(num_online_cpus() * 2)
 
 
-int squashfs_max_decompressors(void)
+static int squashfs_max_decompressors_multi(void)
 {
 	return MAX_DECOMPRESSOR;
 }
 
-
-struct squashfs_stream {
+struct squashfs_stream_multi {
 	void			*comp_opts;
 	struct list_head	strm_list;
 	struct mutex		mutex;
@@ -51,7 +50,7 @@ struct decomp_stream {
 
 
 static void put_decomp_stream(struct decomp_stream *decomp_strm,
-				struct squashfs_stream *stream)
+				struct squashfs_stream_multi *stream)
 {
 	mutex_lock(&stream->mutex);
 	list_add(&decomp_strm->list, &stream->strm_list);
@@ -59,10 +58,10 @@ static void put_decomp_stream(struct decomp_stream *decomp_strm,
 	wake_up(&stream->wait);
 }
 
-void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
+static void *squashfs_decompressor_create_multi(struct squashfs_sb_info *msblk,
 				void *comp_opts)
 {
-	struct squashfs_stream *stream;
+	struct squashfs_stream_multi *stream;
 	struct decomp_stream *decomp_strm = NULL;
 	int err = -ENOMEM;
 
@@ -103,9 +102,9 @@ void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
 }
 
 
-void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
+static void squashfs_decompressor_destroy_multi(struct squashfs_sb_info *msblk)
 {
-	struct squashfs_stream *stream = msblk->stream;
+	struct squashfs_stream_multi *stream = msblk->stream;
 	if (stream) {
 		struct decomp_stream *decomp_strm;
 
@@ -125,7 +124,7 @@ void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
 
 
 static struct decomp_stream *get_decomp_stream(struct squashfs_sb_info *msblk,
-					struct squashfs_stream *stream)
+					struct squashfs_stream_multi *stream)
 {
 	struct decomp_stream *decomp_strm;
 
@@ -180,12 +179,12 @@ static struct decomp_stream *get_decomp_stream(struct squashfs_sb_info *msblk,
 }
 
 
-int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
+static int squashfs_decompress_multi(struct squashfs_sb_info *msblk, struct bio *bio,
 			int offset, int length,
 			struct squashfs_page_actor *output)
 {
 	int res;
-	struct squashfs_stream *stream = msblk->stream;
+	struct squashfs_stream_multi *stream = msblk->stream;
 	struct decomp_stream *decomp_stream = get_decomp_stream(msblk, stream);
 	res = msblk->decompressor->decompress(msblk, decomp_stream->stream,
 		bio, offset, length, output);
@@ -195,3 +194,10 @@ int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
 			msblk->decompressor->name);
 	return res;
 }
+
+const struct squashfs_decompressor_thread_ops squashfs_decompressor_multi = {
+	.create = squashfs_decompressor_create_multi,
+	.destroy = squashfs_decompressor_destroy_multi,
+	.decompress = squashfs_decompress_multi,
+	.max_decompressors = squashfs_max_decompressors_multi,
+};
diff --git a/fs/squashfs/decompressor_multi_percpu.c b/fs/squashfs/decompressor_multi_percpu.c
index b881b9283b7f..e24455a7b04d 100644
--- a/fs/squashfs/decompressor_multi_percpu.c
+++ b/fs/squashfs/decompressor_multi_percpu.c
@@ -20,19 +20,19 @@
  * variables, one thread per cpu core.
  */
 
-struct squashfs_stream {
+struct squashfs_stream_percpu {
 	void			*stream;
 	local_lock_t	lock;
 };
 
-void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
+static void *squashfs_decompressor_create_percpu(struct squashfs_sb_info *msblk,
 						void *comp_opts)
 {
-	struct squashfs_stream *stream;
-	struct squashfs_stream __percpu *percpu;
+	struct squashfs_stream_percpu *stream;
+	struct squashfs_stream_percpu __percpu *percpu;
 	int err, cpu;
 
-	percpu = alloc_percpu(struct squashfs_stream);
+	percpu = alloc_percpu(struct squashfs_stream_percpu);
 	if (percpu == NULL)
 		return ERR_PTR(-ENOMEM);
 
@@ -59,11 +59,11 @@ void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
 	return ERR_PTR(err);
 }
 
-void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
+static void squashfs_decompressor_destroy_percpu(struct squashfs_sb_info *msblk)
 {
-	struct squashfs_stream __percpu *percpu =
-			(struct squashfs_stream __percpu *) msblk->stream;
-	struct squashfs_stream *stream;
+	struct squashfs_stream_percpu __percpu *percpu =
+			(struct squashfs_stream_percpu __percpu *) msblk->stream;
+	struct squashfs_stream_percpu *stream;
 	int cpu;
 
 	if (msblk->stream) {
@@ -75,19 +75,21 @@ void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
 	}
 }
 
-int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
+static int squashfs_decompress_percpu(struct squashfs_sb_info *msblk, struct bio *bio,
 	int offset, int length, struct squashfs_page_actor *output)
 {
-	struct squashfs_stream *stream;
+	struct squashfs_stream_percpu *stream;
+	struct squashfs_stream_percpu __percpu *percpu =
+			(struct squashfs_stream_percpu __percpu *) msblk->stream;
 	int res;
 
-	local_lock(&msblk->stream->lock);
-	stream = this_cpu_ptr(msblk->stream);
+	local_lock(&percpu->lock);
+	stream = this_cpu_ptr(percpu);
 
 	res = msblk->decompressor->decompress(msblk, stream->stream, bio,
 					      offset, length, output);
 
-	local_unlock(&msblk->stream->lock);
+	local_unlock(&percpu->lock);
 
 	if (res < 0)
 		ERROR("%s decompression failed, data probably corrupt\n",
@@ -96,7 +98,14 @@ int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
 	return res;
 }
 
-int squashfs_max_decompressors(void)
+static int squashfs_max_decompressors_percpu(void)
 {
 	return num_possible_cpus();
 }
+
+const struct squashfs_decompressor_thread_ops squashfs_decompressor_percpu = {
+	.create = squashfs_decompressor_create_percpu,
+	.destroy = squashfs_decompressor_destroy_percpu,
+	.decompress = squashfs_decompress_percpu,
+	.max_decompressors = squashfs_max_decompressors_percpu,
+};
diff --git a/fs/squashfs/decompressor_single.c b/fs/squashfs/decompressor_single.c
index 4eb3d083d45e..41449de0ea4c 100644
--- a/fs/squashfs/decompressor_single.c
+++ b/fs/squashfs/decompressor_single.c
@@ -19,15 +19,15 @@
  * decompressor framework
  */
 
-struct squashfs_stream {
+struct squashfs_stream_single {
 	void		*stream;
 	struct mutex	mutex;
 };
 
-void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
+static void *squashfs_decompressor_create_single(struct squashfs_sb_info *msblk,
 						void *comp_opts)
 {
-	struct squashfs_stream *stream;
+	struct squashfs_stream_single *stream;
 	int err = -ENOMEM;
 
 	stream = kmalloc(sizeof(*stream), GFP_KERNEL);
@@ -49,9 +49,9 @@ void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
 	return ERR_PTR(err);
 }
 
-void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
+static void squashfs_decompressor_destroy_single(struct squashfs_sb_info *msblk)
 {
-	struct squashfs_stream *stream = msblk->stream;
+	struct squashfs_stream_single *stream = msblk->stream;
 
 	if (stream) {
 		msblk->decompressor->free(stream->stream);
@@ -59,12 +59,12 @@ void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
 	}
 }
 
-int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
+static int squashfs_decompress_single(struct squashfs_sb_info *msblk, struct bio *bio,
 			int offset, int length,
 			struct squashfs_page_actor *output)
 {
 	int res;
-	struct squashfs_stream *stream = msblk->stream;
+	struct squashfs_stream_single *stream = msblk->stream;
 
 	mutex_lock(&stream->mutex);
 	res = msblk->decompressor->decompress(msblk, stream->stream, bio,
@@ -78,7 +78,14 @@ int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
 	return res;
 }
 
-int squashfs_max_decompressors(void)
+static int squashfs_max_decompressors_single(void)
 {
 	return 1;
 }
+
+const struct squashfs_decompressor_thread_ops squashfs_decompressor_single = {
+	.create = squashfs_decompressor_create_single,
+	.destroy = squashfs_decompressor_destroy_single,
+	.decompress = squashfs_decompress_single,
+	.max_decompressors = squashfs_max_decompressors_single,
+};
diff --git a/fs/squashfs/squashfs.h b/fs/squashfs/squashfs.h
index 9783e01c8100..9a383ad0dae0 100644
--- a/fs/squashfs/squashfs.h
+++ b/fs/squashfs/squashfs.h
@@ -38,12 +38,45 @@ extern const struct squashfs_decompressor *squashfs_lookup_decompressor(int);
 extern void *squashfs_decompressor_setup(struct super_block *, unsigned short);
 
 /* decompressor_xxx.c */
-extern void *squashfs_decompressor_create(struct squashfs_sb_info *, void *);
-extern void squashfs_decompressor_destroy(struct squashfs_sb_info *);
-extern int squashfs_decompress(struct squashfs_sb_info *, struct bio *,
-				int, int, struct squashfs_page_actor *);
-extern int squashfs_max_decompressors(void);
 
+struct squashfs_decompressor_thread_ops {
+	void * (*create)(struct squashfs_sb_info *msblk, void *comp_opts);
+	void (*destroy)(struct squashfs_sb_info *msblk);
+	int (*decompress)(struct squashfs_sb_info *msblk, struct bio *bio,
+			  int offset, int length, struct squashfs_page_actor *output);
+	int (*max_decompressors)(void);
+};
+
+#ifdef CONFIG_SQUASHFS_DECOMP_SINGLE
+extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_single;
+#endif
+#ifdef CONFIG_SQUASHFS_DECOMP_MULTI
+extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_multi;
+#endif
+#ifdef CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU
+extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_percpu;
+#endif
+
+static inline void *squashfs_decompressor_create(struct squashfs_sb_info *msblk, void *comp_opts)
+{
+	return msblk->thread_ops->create(msblk, comp_opts);
+}
+
+static inline void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
+{
+	msblk->thread_ops->destroy(msblk);
+}
+
+static inline int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
+				      int offset, int length, struct squashfs_page_actor *output)
+{
+	return msblk->thread_ops->decompress(msblk, bio, offset, length, output);
+}
+
+static inline int squashfs_max_decompressors(struct squashfs_sb_info *msblk)
+{
+	return msblk->thread_ops->max_decompressors();
+}
 /* export.c */
 extern __le64 *squashfs_read_inode_lookup_table(struct super_block *, u64, u64,
 				unsigned int);
diff --git a/fs/squashfs/squashfs_fs_sb.h b/fs/squashfs/squashfs_fs_sb.h
index 1e90c2575f9b..f1e5dad8ae0a 100644
--- a/fs/squashfs/squashfs_fs_sb.h
+++ b/fs/squashfs/squashfs_fs_sb.h
@@ -53,7 +53,7 @@ struct squashfs_sb_info {
 	__le64					*xattr_id_table;
 	struct mutex				meta_index_mutex;
 	struct meta_index			*meta_index;
-	struct squashfs_stream			*stream;
+	void					*stream;
 	__le64					*inode_lookup_table;
 	u64					inode_table;
 	u64					directory_table;
@@ -66,5 +66,6 @@ struct squashfs_sb_info {
 	int					xattr_ids;
 	unsigned int				ids;
 	bool					panic_on_errors;
+	const struct squashfs_decompressor_thread_ops *thread_ops;
 };
 #endif
diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
index 32565dafa7f3..fd4e70d45f3c 100644
--- a/fs/squashfs/super.c
+++ b/fs/squashfs/super.c
@@ -47,10 +47,12 @@ enum Opt_errors {
 
 enum squashfs_param {
 	Opt_errors,
+	Opt_threads,
 };
 
 struct squashfs_mount_opts {
 	enum Opt_errors errors;
+	const struct squashfs_decompressor_thread_ops *thread_ops;
 };
 
 static const struct constant_table squashfs_param_errors[] = {
@@ -61,9 +63,29 @@ static const struct constant_table squashfs_param_errors[] = {
 
 static const struct fs_parameter_spec squashfs_fs_parameters[] = {
 	fsparam_enum("errors", Opt_errors, squashfs_param_errors),
+	fsparam_string("threads", Opt_threads),
 	{}
 };
 
+static int squashfs_parse_param_threads(const char *str, struct squashfs_mount_opts *opts)
+{
+#ifdef CONFIG_SQUASHFS_CHOICE_DECOMP_BY_MOUNT
+	if (strcmp(str, "single") == 0) {
+		opts->thread_ops = &squashfs_decompressor_single;
+		return 0;
+	}
+	if (strcmp(str, "multi") == 0) {
+		opts->thread_ops = &squashfs_decompressor_multi;
+		return 0;
+	}
+	if (strcmp(str, "percpu") == 0) {
+		opts->thread_ops = &squashfs_decompressor_percpu;
+		return 0;
+	}
+#endif
+	return -EINVAL;
+}
+
 static int squashfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 {
 	struct squashfs_mount_opts *opts = fc->fs_private;
@@ -78,6 +100,10 @@ static int squashfs_parse_param(struct fs_context *fc, struct fs_parameter *para
 	case Opt_errors:
 		opts->errors = result.uint_32;
 		break;
+	case Opt_threads:
+		if (squashfs_parse_param_threads(param->string, opts) != 0)
+			return -EINVAL;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -167,6 +193,7 @@ static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
 			       sb->s_bdev);
 		goto failed_mount;
 	}
+	msblk->thread_ops = opts->thread_ops;
 
 	/* Check the MAJOR & MINOR versions and lookup compression type */
 	msblk->decompressor = supported_squashfs_filesystem(
@@ -252,7 +279,7 @@ static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
 
 	/* Allocate read_page block */
 	msblk->read_page = squashfs_cache_init("data",
-		squashfs_max_decompressors(), msblk->block_size);
+		squashfs_max_decompressors(msblk), msblk->block_size);
 	if (msblk->read_page == NULL) {
 		errorf(fc, "Failed to allocate read_page block");
 		goto failed_mount;
@@ -435,6 +462,20 @@ static int squashfs_show_options(struct seq_file *s, struct dentry *root)
 	else
 		seq_puts(s, ",errors=continue");
 
+#ifdef CONFIG_SQUASHFS_CHOICE_DECOMP_BY_MOUNT
+	if (msblk->thread_ops == &squashfs_decompressor_single) {
+		seq_puts(s, ",threads=single");
+		return 0;
+	}
+	if (msblk->thread_ops == &squashfs_decompressor_multi) {
+		seq_puts(s, ",threads=multi");
+		return 0;
+	}
+	if (msblk->thread_ops == &squashfs_decompressor_percpu) {
+		seq_puts(s, ",threads=percpu");
+		return 0;
+	}
+#endif
 	return 0;
 }
 
@@ -446,6 +487,13 @@ static int squashfs_init_fs_context(struct fs_context *fc)
 	if (!opts)
 		return -ENOMEM;
 
+#ifdef CONFIG_SQUASHFS_DECOMP_SINGLE
+	opts->thread_ops = &squashfs_decompressor_single;
+#elif CONFIG_SQUASHFS_DECOMP_MULTI
+	opts->thread_ops = &squashfs_decompressor_multi;
+#elif CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU
+	opts->thread_ops = &squashfs_decompressor_percpu;
+#endif
 	fc->fs_private = opts;
 	fc->ops = &squashfs_context_ops;
 	return 0;
-- 
2.12.3
Re: [PATCH v3 1/2] squashfs: add the mount parameter theads=<single|multi|percpu>
Posted by Phillip Lougher 3 years, 6 months ago
Review comments below.

Xiaoming Ni <nixiaoming@huawei.com> wrote:
>Squashfs supports three decompression concurrency modes:
>	Single-thread mode: concurrent reads are blocked and the memory overhead
>is small.

Please wrap over 80 column line.

>	Multi-thread mode/percpu mode: reduces concurrent read blocking but
>increases memory overhead.
>
>The corresponding schema must be fixed at compile time. During mounting,
>the concurrent decompression mode cannot be adjusted based on file read
>blocking.
>
>The mount parameter theads=<single|multi|percpu> is added to select
>the concurrent decompression mode of a single SquashFS file system
>image.
>
>Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>---
> fs/squashfs/Kconfig                     | 40 ++++++++++++++++++++++----
> fs/squashfs/decompressor_multi.c        | 28 ++++++++++--------
> fs/squashfs/decompressor_multi_percpu.c | 39 +++++++++++++++----------
> fs/squashfs/decompressor_single.c       | 23 +++++++++------
> fs/squashfs/squashfs.h                  | 43 ++++++++++++++++++++++++----
> fs/squashfs/squashfs_fs_sb.h            |  3 +-
> fs/squashfs/super.c                     | 50 ++++++++++++++++++++++++++++++++-
> 7 files changed, 180 insertions(+), 46 deletions(-)
>
>diff --git a/fs/squashfs/Kconfig b/fs/squashfs/Kconfig
>index 916e78fabcaa..9c2827459f40 100644
>--- a/fs/squashfs/Kconfig
>+++ b/fs/squashfs/Kconfig
>@@ -54,9 +54,37 @@ config SQUASHFS_FILE_DIRECT
> 
> endchoice
> 
>+config SQUASHFS_DECOMP_SINGLE
>+	depends on SQUASHFS
>+	def_bool n
>+
>+config SQUASHFS_DECOMP_MULTI
>+	depends on SQUASHFS
>+	def_bool n
>+
>+config SQUASHFS_DECOMP_MULTI_PERCPU
>+	depends on SQUASHFS
>+	def_bool n
>+
>+config SQUASHFS_CHOICE_DECOMP_BY_MOUNT
>+	bool "Select the parallel decompression mode during mount"
>+	depends on SQUASHFS
>+	default n
>+	select SQUASHFS_DECOMP_SINGLE
>+	select SQUASHFS_DECOMP_MULTI
>+	select SQUASHFS_DECOMP_MULTI_PERCPU
>+	help
>+	  Compile all parallel decompression modes and specify the
>+	  decompression mode by setting "threads=" during mount.
>+
>+	  	  threads=<single|multi|percpu>

git am reports "warning: 1 line adds whitespace errors.", which is here.

[SNIP]

>diff --git a/fs/squashfs/decompressor_multi.c b/fs/squashfs/decompressor_multi.c
>index db9f12a3ea05..7b2723b77e75 100644
>--- a/fs/squashfs/decompressor_multi.c
>+++ b/fs/squashfs/decompressor_multi.c
>@@ -29,13 +29,12 @@
> #define MAX_DECOMPRESSOR	(num_online_cpus() * 2)
> 
> 
>-int squashfs_max_decompressors(void)
>+static int squashfs_max_decompressors_multi(void)

Changing the name of the function *should* be unnecessary, because
you're making it static.

> {
> 	return MAX_DECOMPRESSOR;
> }
> 
>-
>-struct squashfs_stream {
>+struct squashfs_stream_multi {

Again changing the name of the structure should be unnecessary.

This is just extra noise.

The current "diffstat" for the 3 decompression threading implementations is:

fs/squashfs/decompressor_multi.c        | 28 ++++++++++--------
fs/squashfs/decompressor_multi_percpu.c | 39 +++++++++++++++----------
fs/squashfs/decompressor_single.c       | 23 +++++++++------

But, once you get rid of this noise, it drops to

fs/squashfs/decompressor_multi.c        | 16 +++++---
fs/squashfs/decompressor_multi_percpu.c | 23 +++++++----
fs/squashfs/decompressor_single.c       | 15 ++++++--

[SNIP]

>--- a/fs/squashfs/squashfs.h
>+++ b/fs/squashfs/squashfs.h
>@@ -38,12 +38,45 @@ extern const struct squashfs_decompressor *squashfs_lookup_decompressor(int);
> extern void *squashfs_decompressor_setup(struct super_block *, unsigned short);
> 
> /* decompressor_xxx.c */

[SNIP]

>+
>+static inline void *squashfs_decompressor_create(struct squashfs_sb_info *msblk, void *comp_opts)
>+{
>+	return msblk->thread_ops->create(msblk, comp_opts);
>+}
>+
>+static inline void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
>+{
>+	msblk->thread_ops->destroy(msblk);
>+}
>+
>+static inline int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
>+				      int offset, int length, struct squashfs_page_actor *output)
>+{
>+	return msblk->thread_ops->decompress(msblk, bio, offset, length, output);
>+}
>+
>+static inline int squashfs_max_decompressors(struct squashfs_sb_info *msblk)
>+{
>+	return msblk->thread_ops->max_decompressors();
>+}

The above is the reason why you've had to rename the functions in the
decompression threading implementations.  Because you've put the new
accessors into squashfs.h which is also included by the threading
implementations which causes a name clash.

But, the correct way to avoid this clash, is to put the accessors into
a new header file, which isn't included by the decompression threading
implementations.

The following patch does this cleanup, to simplify the patch here.  It also
moves the struct squashfs_decompressor_thread_ops definition into
decompressor.h which is a better place.

Phillip

 fs/squashfs/block.c                     |  1 +
 fs/squashfs/decompressor.c              |  1 +
 fs/squashfs/decompressor.h              |  8 +++++
 fs/squashfs/decompressor_multi.c        | 28 ++++++++---------
 fs/squashfs/decompressor_multi_percpu.c | 36 +++++++++++-----------
 fs/squashfs/decompressor_single.c       | 24 +++++++--------
 fs/squashfs/squashfs.h                  | 40 -------------------------
 fs/squashfs/super.c                     |  1 +
 fs/squashfs/threading.h                 | 30 +++++++++++++++++++
 9 files changed, 85 insertions(+), 84 deletions(-)
 create mode 100644 fs/squashfs/threading.h

diff --git a/fs/squashfs/block.c b/fs/squashfs/block.c
index 833aca92301f..9bcf7e1b64be 100644
--- a/fs/squashfs/block.c
+++ b/fs/squashfs/block.c
@@ -26,6 +26,7 @@
 #include "squashfs.h"
 #include "decompressor.h"
 #include "page_actor.h"
+#include "threading.h"
 
 /*
  * Returns the amount of bytes copied to the page actor.
diff --git a/fs/squashfs/decompressor.c b/fs/squashfs/decompressor.c
index d57bef91ab08..5c355fca0df5 100644
--- a/fs/squashfs/decompressor.c
+++ b/fs/squashfs/decompressor.c
@@ -18,6 +18,7 @@
 #include "decompressor.h"
 #include "squashfs.h"
 #include "page_actor.h"
+#include "threading.h"
 
 /*
  * This file (and decompressor.h) implements a decompressor framework for
diff --git a/fs/squashfs/decompressor.h b/fs/squashfs/decompressor.h
index 19ab60834389..5b5ec4828930 100644
--- a/fs/squashfs/decompressor.h
+++ b/fs/squashfs/decompressor.h
@@ -24,6 +24,14 @@ struct squashfs_decompressor {
 	int	supported;
 };
 
+struct squashfs_decompressor_thread_ops {
+	void * (*create)(struct squashfs_sb_info *msblk, void *comp_opts);
+	void (*destroy)(struct squashfs_sb_info *msblk);
+	int (*decompress)(struct squashfs_sb_info *msblk, struct bio *bio,
+			  int offset, int length, struct squashfs_page_actor *output);
+	int (*max_decompressors)(void);
+};
+
 static inline void *squashfs_comp_opts(struct squashfs_sb_info *msblk,
 							void *buff, int length)
 {
diff --git a/fs/squashfs/decompressor_multi.c b/fs/squashfs/decompressor_multi.c
index 7b2723b77e75..eb25432bd149 100644
--- a/fs/squashfs/decompressor_multi.c
+++ b/fs/squashfs/decompressor_multi.c
@@ -29,12 +29,12 @@
 #define MAX_DECOMPRESSOR	(num_online_cpus() * 2)
 
 
-static int squashfs_max_decompressors_multi(void)
+static int squashfs_max_decompressors(void)
 {
 	return MAX_DECOMPRESSOR;
 }
 
-struct squashfs_stream_multi {
+struct squashfs_stream {
 	void			*comp_opts;
 	struct list_head	strm_list;
 	struct mutex		mutex;
@@ -50,7 +50,7 @@ struct decomp_stream {
 
 
 static void put_decomp_stream(struct decomp_stream *decomp_strm,
-				struct squashfs_stream_multi *stream)
+				struct squashfs_stream *stream)
 {
 	mutex_lock(&stream->mutex);
 	list_add(&decomp_strm->list, &stream->strm_list);
@@ -58,10 +58,10 @@ static void put_decomp_stream(struct decomp_stream *decomp_strm,
 	wake_up(&stream->wait);
 }
 
-static void *squashfs_decompressor_create_multi(struct squashfs_sb_info *msblk,
+static void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
 				void *comp_opts)
 {
-	struct squashfs_stream_multi *stream;
+	struct squashfs_stream *stream;
 	struct decomp_stream *decomp_strm = NULL;
 	int err = -ENOMEM;
 
@@ -102,9 +102,9 @@ static void *squashfs_decompressor_create_multi(struct squashfs_sb_info *msblk,
 }
 
 
-static void squashfs_decompressor_destroy_multi(struct squashfs_sb_info *msblk)
+static void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
 {
-	struct squashfs_stream_multi *stream = msblk->stream;
+	struct squashfs_stream *stream = msblk->stream;
 	if (stream) {
 		struct decomp_stream *decomp_strm;
 
@@ -124,7 +124,7 @@ static void squashfs_decompressor_destroy_multi(struct squashfs_sb_info *msblk)
 
 
 static struct decomp_stream *get_decomp_stream(struct squashfs_sb_info *msblk,
-					struct squashfs_stream_multi *stream)
+					struct squashfs_stream *stream)
 {
 	struct decomp_stream *decomp_strm;
 
@@ -179,12 +179,12 @@ static struct decomp_stream *get_decomp_stream(struct squashfs_sb_info *msblk,
 }
 
 
-static int squashfs_decompress_multi(struct squashfs_sb_info *msblk, struct bio *bio,
+static int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
 			int offset, int length,
 			struct squashfs_page_actor *output)
 {
 	int res;
-	struct squashfs_stream_multi *stream = msblk->stream;
+	struct squashfs_stream *stream = msblk->stream;
 	struct decomp_stream *decomp_stream = get_decomp_stream(msblk, stream);
 	res = msblk->decompressor->decompress(msblk, decomp_stream->stream,
 		bio, offset, length, output);
@@ -196,8 +196,8 @@ static int squashfs_decompress_multi(struct squashfs_sb_info *msblk, struct bio
 }
 
 const struct squashfs_decompressor_thread_ops squashfs_decompressor_multi = {
-	.create = squashfs_decompressor_create_multi,
-	.destroy = squashfs_decompressor_destroy_multi,
-	.decompress = squashfs_decompress_multi,
-	.max_decompressors = squashfs_max_decompressors_multi,
+	.create = squashfs_decompressor_create,
+	.destroy = squashfs_decompressor_destroy,
+	.decompress = squashfs_decompress,
+	.max_decompressors = squashfs_max_decompressors,
 };
diff --git a/fs/squashfs/decompressor_multi_percpu.c b/fs/squashfs/decompressor_multi_percpu.c
index e24455a7b04d..1dfadf76ed9a 100644
--- a/fs/squashfs/decompressor_multi_percpu.c
+++ b/fs/squashfs/decompressor_multi_percpu.c
@@ -20,19 +20,19 @@
  * variables, one thread per cpu core.
  */
 
-struct squashfs_stream_percpu {
+struct squashfs_stream {
 	void			*stream;
 	local_lock_t	lock;
 };
 
-static void *squashfs_decompressor_create_percpu(struct squashfs_sb_info *msblk,
+static void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
 						void *comp_opts)
 {
-	struct squashfs_stream_percpu *stream;
-	struct squashfs_stream_percpu __percpu *percpu;
+	struct squashfs_stream *stream;
+	struct squashfs_stream __percpu *percpu;
 	int err, cpu;
 
-	percpu = alloc_percpu(struct squashfs_stream_percpu);
+	percpu = alloc_percpu(struct squashfs_stream);
 	if (percpu == NULL)
 		return ERR_PTR(-ENOMEM);
 
@@ -59,11 +59,11 @@ static void *squashfs_decompressor_create_percpu(struct squashfs_sb_info *msblk,
 	return ERR_PTR(err);
 }
 
-static void squashfs_decompressor_destroy_percpu(struct squashfs_sb_info *msblk)
+static void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
 {
-	struct squashfs_stream_percpu __percpu *percpu =
-			(struct squashfs_stream_percpu __percpu *) msblk->stream;
-	struct squashfs_stream_percpu *stream;
+	struct squashfs_stream __percpu *percpu =
+			(struct squashfs_stream __percpu *) msblk->stream;
+	struct squashfs_stream *stream;
 	int cpu;
 
 	if (msblk->stream) {
@@ -75,12 +75,12 @@ static void squashfs_decompressor_destroy_percpu(struct squashfs_sb_info *msblk)
 	}
 }
 
-static int squashfs_decompress_percpu(struct squashfs_sb_info *msblk, struct bio *bio,
+static int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
 	int offset, int length, struct squashfs_page_actor *output)
 {
-	struct squashfs_stream_percpu *stream;
-	struct squashfs_stream_percpu __percpu *percpu =
-			(struct squashfs_stream_percpu __percpu *) msblk->stream;
+	struct squashfs_stream *stream;
+	struct squashfs_stream __percpu *percpu =
+			(struct squashfs_stream __percpu *) msblk->stream;
 	int res;
 
 	local_lock(&percpu->lock);
@@ -98,14 +98,14 @@ static int squashfs_decompress_percpu(struct squashfs_sb_info *msblk, struct bio
 	return res;
 }
 
-static int squashfs_max_decompressors_percpu(void)
+static int squashfs_max_decompressors(void)
 {
 	return num_possible_cpus();
 }
 
 const struct squashfs_decompressor_thread_ops squashfs_decompressor_percpu = {
-	.create = squashfs_decompressor_create_percpu,
-	.destroy = squashfs_decompressor_destroy_percpu,
-	.decompress = squashfs_decompress_percpu,
-	.max_decompressors = squashfs_max_decompressors_percpu,
+	.create = squashfs_decompressor_create,
+	.destroy = squashfs_decompressor_destroy,
+	.decompress = squashfs_decompress,
+	.max_decompressors = squashfs_max_decompressors,
 };
diff --git a/fs/squashfs/decompressor_single.c b/fs/squashfs/decompressor_single.c
index 41449de0ea4c..6f161887710b 100644
--- a/fs/squashfs/decompressor_single.c
+++ b/fs/squashfs/decompressor_single.c
@@ -19,15 +19,15 @@
  * decompressor framework
  */
 
-struct squashfs_stream_single {
+struct squashfs_stream {
 	void		*stream;
 	struct mutex	mutex;
 };
 
-static void *squashfs_decompressor_create_single(struct squashfs_sb_info *msblk,
+static void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
 						void *comp_opts)
 {
-	struct squashfs_stream_single *stream;
+	struct squashfs_stream *stream;
 	int err = -ENOMEM;
 
 	stream = kmalloc(sizeof(*stream), GFP_KERNEL);
@@ -49,9 +49,9 @@ static void *squashfs_decompressor_create_single(struct squashfs_sb_info *msblk,
 	return ERR_PTR(err);
 }
 
-static void squashfs_decompressor_destroy_single(struct squashfs_sb_info *msblk)
+static void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
 {
-	struct squashfs_stream_single *stream = msblk->stream;
+	struct squashfs_stream *stream = msblk->stream;
 
 	if (stream) {
 		msblk->decompressor->free(stream->stream);
@@ -59,12 +59,12 @@ static void squashfs_decompressor_destroy_single(struct squashfs_sb_info *msblk)
 	}
 }
 
-static int squashfs_decompress_single(struct squashfs_sb_info *msblk, struct bio *bio,
+static int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
 			int offset, int length,
 			struct squashfs_page_actor *output)
 {
 	int res;
-	struct squashfs_stream_single *stream = msblk->stream;
+	struct squashfs_stream *stream = msblk->stream;
 
 	mutex_lock(&stream->mutex);
 	res = msblk->decompressor->decompress(msblk, stream->stream, bio,
@@ -78,14 +78,14 @@ static int squashfs_decompress_single(struct squashfs_sb_info *msblk, struct bio
 	return res;
 }
 
-static int squashfs_max_decompressors_single(void)
+static int squashfs_max_decompressors(void)
 {
 	return 1;
 }
 
 const struct squashfs_decompressor_thread_ops squashfs_decompressor_single = {
-	.create = squashfs_decompressor_create_single,
-	.destroy = squashfs_decompressor_destroy_single,
-	.decompress = squashfs_decompress_single,
-	.max_decompressors = squashfs_max_decompressors_single,
+	.create = squashfs_decompressor_create,
+	.destroy = squashfs_decompressor_destroy,
+	.decompress = squashfs_decompress,
+	.max_decompressors = squashfs_max_decompressors,
 };
diff --git a/fs/squashfs/squashfs.h b/fs/squashfs/squashfs.h
index 9a383ad0dae0..9ba9e95440f8 100644
--- a/fs/squashfs/squashfs.h
+++ b/fs/squashfs/squashfs.h
@@ -37,46 +37,6 @@ extern void *squashfs_read_table(struct super_block *, u64, int);
 extern const struct squashfs_decompressor *squashfs_lookup_decompressor(int);
 extern void *squashfs_decompressor_setup(struct super_block *, unsigned short);
 
-/* decompressor_xxx.c */
-
-struct squashfs_decompressor_thread_ops {
-	void * (*create)(struct squashfs_sb_info *msblk, void *comp_opts);
-	void (*destroy)(struct squashfs_sb_info *msblk);
-	int (*decompress)(struct squashfs_sb_info *msblk, struct bio *bio,
-			  int offset, int length, struct squashfs_page_actor *output);
-	int (*max_decompressors)(void);
-};
-
-#ifdef CONFIG_SQUASHFS_DECOMP_SINGLE
-extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_single;
-#endif
-#ifdef CONFIG_SQUASHFS_DECOMP_MULTI
-extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_multi;
-#endif
-#ifdef CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU
-extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_percpu;
-#endif
-
-static inline void *squashfs_decompressor_create(struct squashfs_sb_info *msblk, void *comp_opts)
-{
-	return msblk->thread_ops->create(msblk, comp_opts);
-}
-
-static inline void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
-{
-	msblk->thread_ops->destroy(msblk);
-}
-
-static inline int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
-				      int offset, int length, struct squashfs_page_actor *output)
-{
-	return msblk->thread_ops->decompress(msblk, bio, offset, length, output);
-}
-
-static inline int squashfs_max_decompressors(struct squashfs_sb_info *msblk)
-{
-	return msblk->thread_ops->max_decompressors();
-}
 /* export.c */
 extern __le64 *squashfs_read_inode_lookup_table(struct super_block *, u64, u64,
 				unsigned int);
diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
index fd4e70d45f3c..d7dc4b304aa0 100644
--- a/fs/squashfs/super.c
+++ b/fs/squashfs/super.c
@@ -36,6 +36,7 @@
 #include "squashfs.h"
 #include "decompressor.h"
 #include "xattr.h"
+#include "threading.h"
 
 static struct file_system_type squashfs_fs_type;
 static const struct super_operations squashfs_super_ops;
diff --git a/fs/squashfs/threading.h b/fs/squashfs/threading.h
new file mode 100644
index 000000000000..bd06519fb8b9
--- /dev/null
+++ b/fs/squashfs/threading.h
@@ -0,0 +1,30 @@
+#ifdef CONFIG_SQUASHFS_DECOMP_SINGLE
+extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_single;
+#endif
+#ifdef CONFIG_SQUASHFS_DECOMP_MULTI
+extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_multi;
+#endif
+#ifdef CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU
+extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_percpu;
+#endif
+
+static inline void *squashfs_decompressor_create(struct squashfs_sb_info *msblk, void *comp_opts)
+{
+	return msblk->thread_ops->create(msblk, comp_opts);
+}
+
+static inline void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
+{
+	msblk->thread_ops->destroy(msblk);
+}
+
+static inline int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
+				      int offset, int length, struct squashfs_page_actor *output)
+{
+	return msblk->thread_ops->decompress(msblk, bio, offset, length, output);
+}
+
+static inline int squashfs_max_decompressors(struct squashfs_sb_info *msblk)
+{
+	return msblk->thread_ops->max_decompressors();
+}
-- 
2.35.1
Re: [PATCH v3 1/2] squashfs: add the mount parameter theads=<single|multi|percpu>
Posted by Xiaoming Ni 3 years, 6 months ago
On 2022/9/9 23:44, Phillip Lougher wrote:
> Review comments below.
> 
> Xiaoming Ni <nixiaoming@huawei.com> wrote:
>> Squashfs supports three decompression concurrency modes:
>> 	Single-thread mode: concurrent reads are blocked and the memory overhead
>> is small.
> 
> Please wrap over 80 column line.
Thanks for your review, I'll fix it in the next patch version

...
>> +	  Compile all parallel decompression modes and specify the
>> +	  decompression mode by setting "threads=" during mount.
>> +
>> +	  	  threads=<single|multi|percpu>
> 
> git am reports "warning: 1 line adds whitespace errors.", which is here.

Thanks for your review, I'll fix it in the next patch version

> [SNIP]
> 
>> diff --git a/fs/squashfs/decompressor_multi.c b/fs/squashfs/decompressor_multi.c
>> index db9f12a3ea05..7b2723b77e75 100644
>> --- a/fs/squashfs/decompressor_multi.c
>> +++ b/fs/squashfs/decompressor_multi.c
>> @@ -29,13 +29,12 @@
>> #define MAX_DECOMPRESSOR	(num_online_cpus() * 2)
>>
>>
>> -int squashfs_max_decompressors(void)
>> +static int squashfs_max_decompressors_multi(void)
> 
> Changing the name of the function *should* be unnecessary, because
> you're making it static.
> 
>> {
>> 	return MAX_DECOMPRESSOR;
>> }
>>
>> -
>> -struct squashfs_stream {
>> +struct squashfs_stream_multi {
> 
> Again changing the name of the structure should be unnecessary.
> 
> This is just extra noise.
> 
> The current "diffstat" for the 3 decompression threading implementations is:
> 
> fs/squashfs/decompressor_multi.c        | 28 ++++++++++--------
> fs/squashfs/decompressor_multi_percpu.c | 39 +++++++++++++++----------
> fs/squashfs/decompressor_single.c       | 23 +++++++++------
> 
> But, once you get rid of this noise, it drops to
> 
> fs/squashfs/decompressor_multi.c        | 16 +++++---
> fs/squashfs/decompressor_multi_percpu.c | 23 +++++++----
> fs/squashfs/decompressor_single.c       | 15 ++++++--
> 
> [SNIP]
Thanks for your comment, I'll use it in the next patch version to reduce 
the amount of code changes.

> 
>> --- a/fs/squashfs/squashfs.h
>> +++ b/fs/squashfs/squashfs.h
>> @@ -38,12 +38,45 @@ extern const struct squashfs_decompressor *squashfs_lookup_decompressor(int);
>> extern void *squashfs_decompressor_setup(struct super_block *, unsigned short);
>>
>> /* decompressor_xxx.c */
> 
> [SNIP]
> 
>> +
>> +static inline void *squashfs_decompressor_create(struct squashfs_sb_info *msblk, void *comp_opts)
>> +{
>> +	return msblk->thread_ops->create(msblk, comp_opts);
>> +}
>> +
>> +static inline void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
>> +{
>> +	msblk->thread_ops->destroy(msblk);
>> +}
>> +
>> +static inline int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
>> +				      int offset, int length, struct squashfs_page_actor *output)
>> +{
>> +	return msblk->thread_ops->decompress(msblk, bio, offset, length, output);
>> +}
>> +
>> +static inline int squashfs_max_decompressors(struct squashfs_sb_info *msblk)
>> +{
>> +	return msblk->thread_ops->max_decompressors();
>> +}
> 
> The above is the reason why you've had to rename the functions in the
> decompression threading implementations.  Because you've put the new
> accessors into squashfs.h which is also included by the threading
> implementations which causes a name clash.
> 
> But, the correct way to avoid this clash, is to put the accessors into
> a new header file, which isn't included by the decompression threading
> implementations.
> 
> The following patch does this cleanup, to simplify the patch here.  It also
> moves the struct squashfs_decompressor_thread_ops definition into
> decompressor.h which is a better place.
> 
> Phillip
> 
>   fs/squashfs/block.c                     |  1 +
>   fs/squashfs/decompressor.c              |  1 +
>   fs/squashfs/decompressor.h              |  8 +++++
>   fs/squashfs/decompressor_multi.c        | 28 ++++++++---------
>   fs/squashfs/decompressor_multi_percpu.c | 36 +++++++++++-----------
>   fs/squashfs/decompressor_single.c       | 24 +++++++--------
>   fs/squashfs/squashfs.h                  | 40 -------------------------
>   fs/squashfs/super.c                     |  1 +
>   fs/squashfs/threading.h                 | 30 +++++++++++++++++++
>   9 files changed, 85 insertions(+), 84 deletions(-)
>   create mode 100644 fs/squashfs/threading.h

I re-checked the use of create(), destory(), decompress(), 
max_decompressors() and found that the corresponding function had only 
one or two call points, so do we not need to encapsulate in .h?

Squashfs_decompress() is called only once in fs/squashfs/block.c, 
squashfs_read_data().
Squashfs_decompressor_create() is called only once in 
fs/squashfs/decompressor.c, squashfs_decompressor_setup().
squashfs_max_decompressors() is called only once in fs/squashfs/super.c, 
squashfs_fill_super().
squashfs_decompressor_destroy() is called only in fs/squashfs/super.c, 
squashfs_fill_super() and squashfs_put_super().


Thanks
Xiaoming Ni

> 
> diff --git a/fs/squashfs/block.c b/fs/squashfs/block.c
> index 833aca92301f..9bcf7e1b64be 100644
> --- a/fs/squashfs/block.c
> +++ b/fs/squashfs/block.c
> @@ -26,6 +26,7 @@
>   #include "squashfs.h"
>   #include "decompressor.h"
>   #include "page_actor.h"
> +#include "threading.h"
>   
>   /*
>    * Returns the amount of bytes copied to the page actor.
> diff --git a/fs/squashfs/decompressor.c b/fs/squashfs/decompressor.c
> index d57bef91ab08..5c355fca0df5 100644
> --- a/fs/squashfs/decompressor.c
> +++ b/fs/squashfs/decompressor.c
> @@ -18,6 +18,7 @@
>   #include "decompressor.h"
>   #include "squashfs.h"
>   #include "page_actor.h"
> +#include "threading.h"
>   
>   /*
>    * This file (and decompressor.h) implements a decompressor framework for
> diff --git a/fs/squashfs/decompressor.h b/fs/squashfs/decompressor.h
> index 19ab60834389..5b5ec4828930 100644
> --- a/fs/squashfs/decompressor.h
> +++ b/fs/squashfs/decompressor.h
> @@ -24,6 +24,14 @@ struct squashfs_decompressor {
>   	int	supported;
>   };
>   
> +struct squashfs_decompressor_thread_ops {
> +	void * (*create)(struct squashfs_sb_info *msblk, void *comp_opts);
> +	void (*destroy)(struct squashfs_sb_info *msblk);
> +	int (*decompress)(struct squashfs_sb_info *msblk, struct bio *bio,
> +			  int offset, int length, struct squashfs_page_actor *output);
> +	int (*max_decompressors)(void);
> +};
> +
>   static inline void *squashfs_comp_opts(struct squashfs_sb_info *msblk,
>   							void *buff, int length)
>   {
> diff --git a/fs/squashfs/decompressor_multi.c b/fs/squashfs/decompressor_multi.c
> index 7b2723b77e75..eb25432bd149 100644
> --- a/fs/squashfs/decompressor_multi.c
> +++ b/fs/squashfs/decompressor_multi.c
> @@ -29,12 +29,12 @@
>   #define MAX_DECOMPRESSOR	(num_online_cpus() * 2)
>   
>   
> -static int squashfs_max_decompressors_multi(void)
> +static int squashfs_max_decompressors(void)
>   {
>   	return MAX_DECOMPRESSOR;
>   }
>   
> -struct squashfs_stream_multi {
> +struct squashfs_stream {
>   	void			*comp_opts;
>   	struct list_head	strm_list;
>   	struct mutex		mutex;
> @@ -50,7 +50,7 @@ struct decomp_stream {
>   
>   
>   static void put_decomp_stream(struct decomp_stream *decomp_strm,
> -				struct squashfs_stream_multi *stream)
> +				struct squashfs_stream *stream)
>   {
>   	mutex_lock(&stream->mutex);
>   	list_add(&decomp_strm->list, &stream->strm_list);
> @@ -58,10 +58,10 @@ static void put_decomp_stream(struct decomp_stream *decomp_strm,
>   	wake_up(&stream->wait);
>   }
>   
> -static void *squashfs_decompressor_create_multi(struct squashfs_sb_info *msblk,
> +static void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
>   				void *comp_opts)
>   {
> -	struct squashfs_stream_multi *stream;
> +	struct squashfs_stream *stream;
>   	struct decomp_stream *decomp_strm = NULL;
>   	int err = -ENOMEM;
>   
> @@ -102,9 +102,9 @@ static void *squashfs_decompressor_create_multi(struct squashfs_sb_info *msblk,
>   }
>   
>   
> -static void squashfs_decompressor_destroy_multi(struct squashfs_sb_info *msblk)
> +static void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
>   {
> -	struct squashfs_stream_multi *stream = msblk->stream;
> +	struct squashfs_stream *stream = msblk->stream;
>   	if (stream) {
>   		struct decomp_stream *decomp_strm;
>   
> @@ -124,7 +124,7 @@ static void squashfs_decompressor_destroy_multi(struct squashfs_sb_info *msblk)
>   
>   
>   static struct decomp_stream *get_decomp_stream(struct squashfs_sb_info *msblk,
> -					struct squashfs_stream_multi *stream)
> +					struct squashfs_stream *stream)
>   {
>   	struct decomp_stream *decomp_strm;
>   
> @@ -179,12 +179,12 @@ static struct decomp_stream *get_decomp_stream(struct squashfs_sb_info *msblk,
>   }
>   
>   
> -static int squashfs_decompress_multi(struct squashfs_sb_info *msblk, struct bio *bio,
> +static int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
>   			int offset, int length,
>   			struct squashfs_page_actor *output)
>   {
>   	int res;
> -	struct squashfs_stream_multi *stream = msblk->stream;
> +	struct squashfs_stream *stream = msblk->stream;
>   	struct decomp_stream *decomp_stream = get_decomp_stream(msblk, stream);
>   	res = msblk->decompressor->decompress(msblk, decomp_stream->stream,
>   		bio, offset, length, output);
> @@ -196,8 +196,8 @@ static int squashfs_decompress_multi(struct squashfs_sb_info *msblk, struct bio
>   }
>   
>   const struct squashfs_decompressor_thread_ops squashfs_decompressor_multi = {
> -	.create = squashfs_decompressor_create_multi,
> -	.destroy = squashfs_decompressor_destroy_multi,
> -	.decompress = squashfs_decompress_multi,
> -	.max_decompressors = squashfs_max_decompressors_multi,
> +	.create = squashfs_decompressor_create,
> +	.destroy = squashfs_decompressor_destroy,
> +	.decompress = squashfs_decompress,
> +	.max_decompressors = squashfs_max_decompressors,
>   };
> diff --git a/fs/squashfs/decompressor_multi_percpu.c b/fs/squashfs/decompressor_multi_percpu.c
> index e24455a7b04d..1dfadf76ed9a 100644
> --- a/fs/squashfs/decompressor_multi_percpu.c
> +++ b/fs/squashfs/decompressor_multi_percpu.c
> @@ -20,19 +20,19 @@
>    * variables, one thread per cpu core.
>    */
>   
> -struct squashfs_stream_percpu {
> +struct squashfs_stream {
>   	void			*stream;
>   	local_lock_t	lock;
>   };
>   
> -static void *squashfs_decompressor_create_percpu(struct squashfs_sb_info *msblk,
> +static void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
>   						void *comp_opts)
>   {
> -	struct squashfs_stream_percpu *stream;
> -	struct squashfs_stream_percpu __percpu *percpu;
> +	struct squashfs_stream *stream;
> +	struct squashfs_stream __percpu *percpu;
>   	int err, cpu;
>   
> -	percpu = alloc_percpu(struct squashfs_stream_percpu);
> +	percpu = alloc_percpu(struct squashfs_stream);
>   	if (percpu == NULL)
>   		return ERR_PTR(-ENOMEM);
>   
> @@ -59,11 +59,11 @@ static void *squashfs_decompressor_create_percpu(struct squashfs_sb_info *msblk,
>   	return ERR_PTR(err);
>   }
>   
> -static void squashfs_decompressor_destroy_percpu(struct squashfs_sb_info *msblk)
> +static void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
>   {
> -	struct squashfs_stream_percpu __percpu *percpu =
> -			(struct squashfs_stream_percpu __percpu *) msblk->stream;
> -	struct squashfs_stream_percpu *stream;
> +	struct squashfs_stream __percpu *percpu =
> +			(struct squashfs_stream __percpu *) msblk->stream;
> +	struct squashfs_stream *stream;
>   	int cpu;
>   
>   	if (msblk->stream) {
> @@ -75,12 +75,12 @@ static void squashfs_decompressor_destroy_percpu(struct squashfs_sb_info *msblk)
>   	}
>   }
>   
> -static int squashfs_decompress_percpu(struct squashfs_sb_info *msblk, struct bio *bio,
> +static int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
>   	int offset, int length, struct squashfs_page_actor *output)
>   {
> -	struct squashfs_stream_percpu *stream;
> -	struct squashfs_stream_percpu __percpu *percpu =
> -			(struct squashfs_stream_percpu __percpu *) msblk->stream;
> +	struct squashfs_stream *stream;
> +	struct squashfs_stream __percpu *percpu =
> +			(struct squashfs_stream __percpu *) msblk->stream;
>   	int res;
>   
>   	local_lock(&percpu->lock);
> @@ -98,14 +98,14 @@ static int squashfs_decompress_percpu(struct squashfs_sb_info *msblk, struct bio
>   	return res;
>   }
>   
> -static int squashfs_max_decompressors_percpu(void)
> +static int squashfs_max_decompressors(void)
>   {
>   	return num_possible_cpus();
>   }
>   
>   const struct squashfs_decompressor_thread_ops squashfs_decompressor_percpu = {
> -	.create = squashfs_decompressor_create_percpu,
> -	.destroy = squashfs_decompressor_destroy_percpu,
> -	.decompress = squashfs_decompress_percpu,
> -	.max_decompressors = squashfs_max_decompressors_percpu,
> +	.create = squashfs_decompressor_create,
> +	.destroy = squashfs_decompressor_destroy,
> +	.decompress = squashfs_decompress,
> +	.max_decompressors = squashfs_max_decompressors,
>   };
> diff --git a/fs/squashfs/decompressor_single.c b/fs/squashfs/decompressor_single.c
> index 41449de0ea4c..6f161887710b 100644
> --- a/fs/squashfs/decompressor_single.c
> +++ b/fs/squashfs/decompressor_single.c
> @@ -19,15 +19,15 @@
>    * decompressor framework
>    */
>   
> -struct squashfs_stream_single {
> +struct squashfs_stream {
>   	void		*stream;
>   	struct mutex	mutex;
>   };
>   
> -static void *squashfs_decompressor_create_single(struct squashfs_sb_info *msblk,
> +static void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
>   						void *comp_opts)
>   {
> -	struct squashfs_stream_single *stream;
> +	struct squashfs_stream *stream;
>   	int err = -ENOMEM;
>   
>   	stream = kmalloc(sizeof(*stream), GFP_KERNEL);
> @@ -49,9 +49,9 @@ static void *squashfs_decompressor_create_single(struct squashfs_sb_info *msblk,
>   	return ERR_PTR(err);
>   }
>   
> -static void squashfs_decompressor_destroy_single(struct squashfs_sb_info *msblk)
> +static void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
>   {
> -	struct squashfs_stream_single *stream = msblk->stream;
> +	struct squashfs_stream *stream = msblk->stream;
>   
>   	if (stream) {
>   		msblk->decompressor->free(stream->stream);
> @@ -59,12 +59,12 @@ static void squashfs_decompressor_destroy_single(struct squashfs_sb_info *msblk)
>   	}
>   }
>   
> -static int squashfs_decompress_single(struct squashfs_sb_info *msblk, struct bio *bio,
> +static int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
>   			int offset, int length,
>   			struct squashfs_page_actor *output)
>   {
>   	int res;
> -	struct squashfs_stream_single *stream = msblk->stream;
> +	struct squashfs_stream *stream = msblk->stream;
>   
>   	mutex_lock(&stream->mutex);
>   	res = msblk->decompressor->decompress(msblk, stream->stream, bio,
> @@ -78,14 +78,14 @@ static int squashfs_decompress_single(struct squashfs_sb_info *msblk, struct bio
>   	return res;
>   }
>   
> -static int squashfs_max_decompressors_single(void)
> +static int squashfs_max_decompressors(void)
>   {
>   	return 1;
>   }
>   
>   const struct squashfs_decompressor_thread_ops squashfs_decompressor_single = {
> -	.create = squashfs_decompressor_create_single,
> -	.destroy = squashfs_decompressor_destroy_single,
> -	.decompress = squashfs_decompress_single,
> -	.max_decompressors = squashfs_max_decompressors_single,
> +	.create = squashfs_decompressor_create,
> +	.destroy = squashfs_decompressor_destroy,
> +	.decompress = squashfs_decompress,
> +	.max_decompressors = squashfs_max_decompressors,
>   };
> diff --git a/fs/squashfs/squashfs.h b/fs/squashfs/squashfs.h
> index 9a383ad0dae0..9ba9e95440f8 100644
> --- a/fs/squashfs/squashfs.h
> +++ b/fs/squashfs/squashfs.h
> @@ -37,46 +37,6 @@ extern void *squashfs_read_table(struct super_block *, u64, int);
>   extern const struct squashfs_decompressor *squashfs_lookup_decompressor(int);
>   extern void *squashfs_decompressor_setup(struct super_block *, unsigned short);
>   
> -/* decompressor_xxx.c */
> -
> -struct squashfs_decompressor_thread_ops {
> -	void * (*create)(struct squashfs_sb_info *msblk, void *comp_opts);
> -	void (*destroy)(struct squashfs_sb_info *msblk);
> -	int (*decompress)(struct squashfs_sb_info *msblk, struct bio *bio,
> -			  int offset, int length, struct squashfs_page_actor *output);
> -	int (*max_decompressors)(void);
> -};
> -
> -#ifdef CONFIG_SQUASHFS_DECOMP_SINGLE
> -extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_single;
> -#endif
> -#ifdef CONFIG_SQUASHFS_DECOMP_MULTI
> -extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_multi;
> -#endif
> -#ifdef CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU
> -extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_percpu;
> -#endif
> -
> -static inline void *squashfs_decompressor_create(struct squashfs_sb_info *msblk, void *comp_opts)
> -{
> -	return msblk->thread_ops->create(msblk, comp_opts);
> -}
> -
> -static inline void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
> -{
> -	msblk->thread_ops->destroy(msblk);
> -}
> -
> -static inline int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
> -				      int offset, int length, struct squashfs_page_actor *output)
> -{
> -	return msblk->thread_ops->decompress(msblk, bio, offset, length, output);
> -}
> -
> -static inline int squashfs_max_decompressors(struct squashfs_sb_info *msblk)
> -{
> -	return msblk->thread_ops->max_decompressors();
> -}
>   /* export.c */
>   extern __le64 *squashfs_read_inode_lookup_table(struct super_block *, u64, u64,
>   				unsigned int);
> diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
> index fd4e70d45f3c..d7dc4b304aa0 100644
> --- a/fs/squashfs/super.c
> +++ b/fs/squashfs/super.c
> @@ -36,6 +36,7 @@
>   #include "squashfs.h"
>   #include "decompressor.h"
>   #include "xattr.h"
> +#include "threading.h"
>   
>   static struct file_system_type squashfs_fs_type;
>   static const struct super_operations squashfs_super_ops;
> diff --git a/fs/squashfs/threading.h b/fs/squashfs/threading.h
> new file mode 100644
> index 000000000000..bd06519fb8b9
> --- /dev/null
> +++ b/fs/squashfs/threading.h
> @@ -0,0 +1,30 @@
> +#ifdef CONFIG_SQUASHFS_DECOMP_SINGLE
> +extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_single;
> +#endif
> +#ifdef CONFIG_SQUASHFS_DECOMP_MULTI
> +extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_multi;
> +#endif
> +#ifdef CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU
> +extern const struct squashfs_decompressor_thread_ops squashfs_decompressor_percpu;
> +#endif
> +
> +static inline void *squashfs_decompressor_create(struct squashfs_sb_info *msblk, void *comp_opts)
> +{
> +	return msblk->thread_ops->create(msblk, comp_opts);
> +}
> +
> +static inline void squashfs_decompressor_destroy(struct squashfs_sb_info *msblk)
> +{
> +	msblk->thread_ops->destroy(msblk);
> +}
> +
> +static inline int squashfs_decompress(struct squashfs_sb_info *msblk, struct bio *bio,
> +				      int offset, int length, struct squashfs_page_actor *output)
> +{
> +	return msblk->thread_ops->decompress(msblk, bio, offset, length, output);
> +}
> +
> +static inline int squashfs_max_decompressors(struct squashfs_sb_info *msblk)
> +{
> +	return msblk->thread_ops->max_decompressors();
> +}
>
Re: [PATCH v3 1/2] squashfs: add the mount parameter theads=<single|multi|percpu>
Posted by Phillip Lougher 3 years, 6 months ago
On 02/09/2022 10:48, Xiaoming Ni wrote:
> Squashfs supports three decompression concurrency modes:
> 	Single-thread mode: concurrent reads are blocked and the memory overhead
> is small.
> 	Multi-thread mode/percpu mode: reduces concurrent read blocking but
> increases memory overhead.
> 
> The corresponding schema must be fixed at compile time. During mounting,
> the concurrent decompression mode cannot be adjusted based on file read
> blocking.
> 
> The mount parameter theads=<single|multi|percpu> is added to select
> the concurrent decompression mode of a single SquashFS file system
> image.
> 
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>

Additional review comment ...

[SNIP]

> diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
> index 32565dafa7f3..fd4e70d45f3c 100644
> --- a/fs/squashfs/super.c
> +++ b/fs/squashfs/super.c
> @@ -47,10 +47,12 @@ enum Opt_errors {

[SNIP]

> +#ifdef CONFIG_SQUASHFS_DECOMP_SINGLE
> +	opts->thread_ops = &squashfs_decompressor_single; > +#elif CONFIG_SQUASHFS_DECOMP_MULTI

this should be

#elif defined CONFIG_SQUASHFS_DECOMP_MULTI

> +	opts->thread_ops = &squashfs_decompressor_multi;
> +#elif CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU

this should be

#elif defined CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU

Phillip

> +	opts->thread_ops = &squashfs_decompressor_percpu;
> +#endif
>   	fc->fs_private = opts;
>   	fc->ops = &squashfs_context_ops;
>   	return 0;
Re: [PATCH v3 1/2] squashfs: add the mount parameter theads=<single|multi|percpu>
Posted by Xiaoming Ni 3 years, 6 months ago
On 2022/9/9 23:50, Phillip Lougher wrote:
> On 02/09/2022 10:48, Xiaoming Ni wrote:
>> Squashfs supports three decompression concurrency modes:
>>     Single-thread mode: concurrent reads are blocked and the memory 
>> overhead
>> is small.
>>     Multi-thread mode/percpu mode: reduces concurrent read blocking but
>> increases memory overhead.
>>
>> The corresponding schema must be fixed at compile time. During mounting,
>> the concurrent decompression mode cannot be adjusted based on file read
>> blocking.
>>
>> The mount parameter theads=<single|multi|percpu> is added to select
>> the concurrent decompression mode of a single SquashFS file system
>> image.
>>
>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> 
> Additional review comment ...
> 
> [SNIP]
> 
>> diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
>> index 32565dafa7f3..fd4e70d45f3c 100644
>> --- a/fs/squashfs/super.c
>> +++ b/fs/squashfs/super.c
>> @@ -47,10 +47,12 @@ enum Opt_errors {
> 
> [SNIP]
> 
>> +#ifdef CONFIG_SQUASHFS_DECOMP_SINGLE
>> +    opts->thread_ops = &squashfs_decompressor_single; > +#elif 
>> CONFIG_SQUASHFS_DECOMP_MULTI
> 
> this should be
> 
> #elif defined CONFIG_SQUASHFS_DECOMP_MULTI
> 

Thanks for your comment, I'll fix it in the next patch version

>> +    opts->thread_ops = &squashfs_decompressor_multi;
>> +#elif CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU
> 
> this should be
> 
> #elif defined CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU
> 

Thanks for your comment, I'll fix it in the next patch version

> Phillip
> 
>> +    opts->thread_ops = &squashfs_decompressor_percpu;
>> +#endif
>>       fc->fs_private = opts;
>>       fc->ops = &squashfs_context_ops;
>>       return 0;
> 
> 
> .

Thanks
Xiaoming Ni