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

[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@xxxxxxxxxx>
>   
> +#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);