Messages in this thread Patch in this message | | | Date | Wed, 28 Sep 2022 03:20:56 +0100 | Subject | Re: [PATCH v4 1/2] squashfs: add the mount parameter theads=<single|multi|percpu> | From | Phillip Lougher <> |
| |
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);
| |