linux-kernel - Re: ping //Re: [PATCH v2 0/2] squashfs: Add the mount parameter "threads="

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 31 Aug 2022 09:09:34 +0800
From:   Xiaoming Ni <nixiaoming@...wei.com>
To:     Phillip Lougher <phillip@...ashfs.org.uk>,
        <linux-kernel@...r.kernel.org>
CC:     <wangle6@...wei.com>, <yi.zhang@...wei.com>,
        <wangbing6@...wei.com>, <zhongjubin@...wei.com>,
        <chenjianguo3@...wei.com>
Subject: Re: ping //Re: [PATCH v2 0/2] squashfs: Add the mount parameter
 "threads="

On 2022/8/31 2:34, Phillip Lougher wrote:
> 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@...enix:/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
> 
> 
Thanks for your guidance, I will update it in v3 patch as soon as possible

Thanks

Xiaoming Ni