Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b3018490-b951-01f0-2b86-84049d9470a0@squashfs.org.uk>
Date:   Wed, 28 Sep 2022 03:20:56 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH v4 1/2] squashfs: add the mount parameter
 theads=<single|multi|percpu>
To:     Xiaoming Ni <nixiaoming@huawei.com>, linux-kernel@vger.kernel.org
Cc:     wangle6@huawei.com, yi.zhang@huawei.com, wangbing6@huawei.com,
        zhongjubin@huawei.com, chenjianguo3@huawei.com
References: <20220902094855.22666-1-nixiaoming@huawei.com>
 <20220916083604.33408-1-nixiaoming@huawei.com>
 <20220916083604.33408-2-nixiaoming@huawei.com>
From:   Phillip Lougher <phillip@squashfs.org.uk>
In-Reply-To: <20220916083604.33408-2-nixiaoming@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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);