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: <60b24133-234f-858b-8e71-e183fe72d2bb@squashfs.org.uk>
Date:   Mon, 29 Aug 2022 00:18:01 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: ping //Re: [PATCH v2 0/2] squashfs: Add the mount parameter
 "threads="
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: <20220815031100.75243-1-nixiaoming@huawei.com>
 <20220816010052.15764-1-nixiaoming@huawei.com>
 <8d139f03-7845-9c96-fffc-74fdf8b5d78d@huawei.com>
From:   Phillip Lougher <phillip@squashfs.org.uk>
In-Reply-To: <8d139f03-7845-9c96-fffc-74fdf8b5d78d@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 26/08/2022 07:19, Xiaoming Ni wrote:
> ping
> 
> 
> On 2022/8/16 9:00, Xiaoming Ni wrote:
>> Currently, Squashfs supports multiple decompressor parallel modes. 
>> However, this
>> mode can be configured only during kernel building and does not 
>> support flexible
>> selection during runtime.
>>
>> In the current patch set, the mount parameter "threads=" is added to 
>> allow users
>> to select the parallel decompressor mode and configure the number of 
>> decompressors
>> when mounting a file system.
>>
>> v2: fix warning: sparse: incorrect type in initializer (different 
>> address spaces)
>>    Reported-by: kernel test robot <lkp@intel.com>

I have made an initial review of the patches, and I have the following
comments.

Good things about the patch-series.

1. In principle I have no objections to making this configurable at
    mount time.  But, a use-case for why this has become necessary
    would help in the evaluation.

2. In general the code changes are good.  They are predominantly
    exposing the existing different decompressor functionality into
    structures which can be selected at mount time.  They do not
    change existing functionality, and so there are no issues
    about unexpected regressions.

Things which I don't like about the patch-series.

1. There is no default kernel configuration option to keep the existing
    behaviour, that is build time selectable only.  There may be many
    companies/people where for "security" reasons the ability to
    switch to a more CPU/memory intensive decompressor or more threads
    is a risk.

    Yes, I know the new kernel configuration options allow only the
    selected default decompressor mode to be built.  In theory that
    will restrict the available decompressors to the single decompressor
    selected at build time.  So not much different to the current
    position?  But, if the CONFIG_SQUASHFS_DECOMP_MULTI decompressor
    is selected, that will now allow more threads to be used than is
    current, where it is currently restricted to num_online_cpus() * 2.

2. You have decided to allow the mutiple decompressor implementations
    to be selected at mount time - but you have also allowed only one
    decompressor to be built at kernel build time.  This means you
    end up in the fairly silly situation of having a mount time
    option which allows the user to select between one decompressor.
    There doesn't seem much point in having an option which allows
    nothing to be changed.

3. Using thread=<number>, where thread=1 you use SQUASHFS_DECOMP_SINGLE
    if it has been built, otherwise you fall back to
    SQUASHFS_DECOMP_MULTI.  This meants the effect of thread=1 is
    indeterminate and depends on the build options.  I would suggest
    thread=1 should always mean use SQUASHFS_DECOMP_SINGLE.

4. If SQUASHFS_DECOMP_MULTI is selected, there isn't a limit on the
    maximum amount of threads allowed, and there is no ability to
    set the maximum number of threads allowed at kernel build time
    either.

All of the above seems to be a bit of a mess.

As regards points 1 - 3, personally I would add a default kernel
configuration option that keeps the existing behaviour, build time
selectable only, no additional mount time options.  Then a
kernel configuration option that allows the different decompressors
to be selected at mount time, but which always builds all the
decompressors.  This will avoid the silliness of point 2, and
the indeterminate behaviour of point 3.

As regards point 4, I think you should require the maximum number
of threads allowable to be determined at build time, this is
good for security and avoids attempts to use too much CPU
and memory.  The default at kernel build time should be minimal,
to avoid cases where an unchanged value can cause a potential
security hazard on a low end system.  In otherwords it is
up to the user at build time to set the value to an appropriate
value for their system.

Phillip

---
Phillip Lougher, Squashfs author and maintainer.

>>
>> v1: 
>> https://lore.kernel.org/lkml/20220815031100.75243-1-nixiaoming@huawei.com/ 
>>
>> ----
>>
>> Xiaoming Ni (2):
>>    squashfs: add the mount parameter theads=<single|multi|percpu>
>>    squashfs: Allows users to configure the number of decompression
>>      threads.
>>
>>   fs/squashfs/Kconfig                     | 24 ++++++++--
>>   fs/squashfs/decompressor_multi.c        | 32 ++++++++------
>>   fs/squashfs/decompressor_multi_percpu.c | 39 ++++++++++-------
>>   fs/squashfs/decompressor_single.c       | 23 ++++++----
>>   fs/squashfs/squashfs.h                  | 39 ++++++++++++++---
>>   fs/squashfs/squashfs_fs_sb.h            |  4 +-
>>   fs/squashfs/super.c                     | 77 
>> ++++++++++++++++++++++++++++++++-
>>   7 files changed, 192 insertions(+), 46 deletions(-)
>>
>