lkml.org 
[lkml]   [2022]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v4 1/2] squashfs: add the mount parameter theads=<single|multi|percpu>
From
On 16/09/2022 09:36, 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>
>
> +#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

In my previous review I asked you to fix the above two #elif
lines. You have done so in this patch series, but, only in the
second patch which seems perverse.

The reason why this isn't a good approach. If you *only* apply this
patch, with the following Squashfs configuration options

phillip@phoenix:/external/stripe/linux$ grep SQUASHFS .config
CONFIG_SQUASHFS=y
# CONFIG_SQUASHFS_FILE_CACHE is not set
CONFIG_SQUASHFS_FILE_DIRECT=y
CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU=y
# CONFIG_SQUASHFS_CHOICE_DECOMP_BY_MOUNT is not set
# CONFIG_SQUASHFS_COMPILE_DECOMP_SINGLE is not set
# CONFIG_SQUASHFS_COMPILE_DECOMP_MULTI is not set
CONFIG_SQUASHFS_COMPILE_DECOMP_MULTI_PERCPU=y
CONFIG_SQUASHFS_XATTR=y
CONFIG_SQUASHFS_ZLIB=y
CONFIG_SQUASHFS_LZ4=y
CONFIG_SQUASHFS_LZO=y
CONFIG_SQUASHFS_XZ=y
CONFIG_SQUASHFS_ZSTD=y
# CONFIG_SQUASHFS_4K_DEVBLK_SIZE is not set
# CONFIG_SQUASHFS_EMBEDDED is not set
CONFIG_SQUASHFS_FRAGMENT_CACHE_SIZE=3


You will get the following build warning

phillip@phoenix:/external/stripe/linux$ make bzImage
SYNC include/config/auto.conf.cmd
CALL scripts/checksyscalls.sh
CALL scripts/atomic/check-atomics.sh
DESCEND objtool
CHK include/generated/compile.h
UPD kernel/config_data
GZIP kernel/config_data.gz
CC kernel/configs.o
AR kernel/built-in.a
CC fs/squashfs/block.o
CC fs/squashfs/cache.o
CC fs/squashfs/dir.o
CC fs/squashfs/export.o
CC fs/squashfs/file.o
CC fs/squashfs/fragment.o
CC fs/squashfs/id.o
CC fs/squashfs/inode.o
CC fs/squashfs/namei.o
CC fs/squashfs/super.o
fs/squashfs/super.c: In function ‘squashfs_init_fs_context’:
fs/squashfs/super.c:492:7: warning: "CONFIG_SQUASHFS_DECOMP_MULTI" is
not defined, evaluates to 0 [-Wundef]
492 | #elif CONFIG_SQUASHFS_DECOMP_MULTI
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

This strictly breaks the git bisectability rule. Every patch should
be compilable and not break the build or produce warnings. If not
it makes git bisect difficult to use to find regressions.

This can be avoided by fixing the issue in *this* patch. So
please do so.

Thanks

Phillip

> + opts->thread_ops = &squashfs_decompressor_percpu;
> +#endif
> fc->fs_private = opts;
> fc->ops = &squashfs_context_ops;
> return 0;
> @@ -478,7 +526,7 @@ static void squashfs_put_super(struct super_block *sb)
> squashfs_cache_delete(sbi->block_cache);
> squashfs_cache_delete(sbi->fragment_cache);
> squashfs_cache_delete(sbi->read_page);
> - squashfs_decompressor_destroy(sbi);
> + sbi->thread_ops->destroy(sbi);
> kfree(sbi->id_table);
> kfree(sbi->fragment_index);
> kfree(sbi->meta_index);

\
 
 \ /
  Last update: 2022-09-28 04:22    [W:0.154 / U:0.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site