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: <ab006c5e-1729-3e46-0daa-0df43a05a88b@squashfs.org.uk>
Date:   Tue, 30 Aug 2022 19:34:44 +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="
From:   Phillip Lougher <phillip@squashfs.org.uk>
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>
 <60b24133-234f-858b-8e71-e183fe72d2bb@squashfs.org.uk>
 <9c5bddc3-fc93-d76a-1163-0278c8d31dc5@huawei.com>
 <74fb0469-e593-ed0e-3f9b-5ec535969c78@squashfs.org.uk>
In-Reply-To: <74fb0469-e593-ed0e-3f9b-5ec535969c78@squashfs.org.uk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 30/08/2022 19:08, Phillip Lougher wrote:
> On 30/08/2022 14:38, Xiaoming Ni wrote:
>> On 2022/8/29 7:18, Phillip Lougher wrote:

>>> 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
>> Would it be better to allow flexible selection of decompression mode 
>> combinations?
> 
> I told you I don't like that (*).  I also told you I want the default 
> behaviour to be the current behaviour.
> 
> Feel free to disagree, but that isn't a good way to get your patch
> reviewed or accepted by me.
> 
> Cheers
> 
> Phillip
> 
> (*) Adding options to select the decompressor at mount time, but,
>      also allowing only 1 - 2 decompressors to be built is a waste of
>      time.  It has the effect of giving something with one hand and
>      taking it alway with the other.  Build the lot, this also
>      keeps it simple.
> 
> 

It also splatters super.c with #ifdef CONFIG lines.

phillip@phoenix:/external/stripe/linux/fs/squashfs$ grep "CONFIG_" super.c
#ifdef CONFIG_SQUASHFS_DECOMP_SINGLE
#ifdef CONFIG_SQUASHFS_DECOMP_MULTI
#ifdef CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU
#ifdef CONFIG_SQUASHFS_DECOMP_SINGLE
#ifdef CONFIG_SQUASHFS_DECOMP_MULTI
#ifdef CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU
#ifdef CONFIG_SQUASHFS_DECOMP_DEFAULT_SINGLE
#elif defined(CONFIG_SQUASHFS_DECOMP_DEFAULT_MULTI)
#elif defined(CONFIG_SQUASHFS_DECOMP_DEFAULT_PERCPU)

Or

static int squashfs_parse_param_threads(const char *str, struct 
squashfs_mount_opts *opts)
{
         unsigned long num;
         int ret;
#ifdef CONFIG_SQUASHFS_DECOMP_SINGLE
         if (strcmp(str, "single") == 0) {
                 opts->thread_ops = &squashfs_decompressor_single;
                 return 0;
         }
#endif
#ifdef CONFIG_SQUASHFS_DECOMP_MULTI
         if (strcmp(str, "multi") == 0) {
                 opts->thread_ops = &squashfs_decompressor_multi;
                 return 0;
         }
#endif
#ifdef CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU
         if (strcmp(str, "percpu") == 0) {
                 opts->thread_ops = &squashfs_decompressor_percpu;
                 return 0;
         }
#endif
         ret = kstrtoul(str, 0, &num);
         if (ret != 0 || num == 0)
                 return -EINVAL;
#ifdef CONFIG_SQUASHFS_DECOMP_SINGLE
         if (num == 1) {
                 opts->thread_ops = &squashfs_decompressor_single;
                 return 0;
         }
#endif
#ifdef CONFIG_SQUASHFS_DECOMP_MULTI
         opts->thread_ops = &squashfs_decompressor_multi;
         if (num > opts->thread_ops->max_decompressors())
                 num = opts->thread_ops->max_decompressors();
         opts->thread_num = (int)num;
         return 0;
#else
         return -EINVAL;
#endif
}

SNIP

static int squashfs_show_options(struct seq_file *s, struct dentry *root)
{
         struct super_block *sb = root->d_sb;
         struct squashfs_sb_info *msblk = sb->s_fs_info;

         if (msblk->panic_on_errors)
                 seq_puts(s, ",errors=panic");
         else
                 seq_puts(s, ",errors=continue");

#ifdef CONFIG_SQUASHFS_DECOMP_MULTI_PERCPU
         /*
          * thread=percpu and thread=<number> have different 
configuration effects.
          * the parameters need to be treated differently when they are 
displayed.
          */
         if (msblk->thread_ops == &squashfs_decompressor_percpu) {
                 seq_puts(s, ",threads=percpu");
                 return 0;
         }
#endif
         seq_printf(s, ",threads=%u", msblk->max_thread_num);
         return 0;
}

static int squashfs_init_fs_context(struct fs_context *fc)
{
         struct squashfs_mount_opts *opts;

         opts = kzalloc(sizeof(*opts), GFP_KERNEL);
         if (!opts)
                 return -ENOMEM;

#ifdef CONFIG_SQUASHFS_DECOMP_DEFAULT_SINGLE
         opts->thread_ops = &squashfs_decompressor_single;
#elif defined(CONFIG_SQUASHFS_DECOMP_DEFAULT_MULTI)
         opts->thread_ops = &squashfs_decompressor_multi,
#elif defined(CONFIG_SQUASHFS_DECOMP_DEFAULT_PERCPU)
         opts->thread_ops = &squashfs_decompressor_percpu;
#endif