| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 13 Sep 2022 10:46:10 +0800
From: Xiaoming Ni <nixiaoming@...wei.com>
To: Phillip Lougher <phillip@...ashfs.org.uk>
CC: <chenjianguo3@...wei.com>, <linux-kernel@...r.kernel.org>,
<wangbing6@...wei.com>, <wangle6@...wei.com>,
<yi.zhang@...wei.com>, <zhongjubin@...wei.com>
Subject: Re: [PATCH v3 1/2] squashfs: add the mount parameter
theads=<single|multi|percpu>
On 2022/9/9 23:44, Phillip Lougher wrote:
> Review comments below.
>
> Xiaoming Ni <nixiaoming@...wei.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();
> +}
>
Powered by blists - more mailing lists