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

From: Xiaoming Ni
Date: Tue Aug 30 2022 - 09:40:51 EST

On 2022/8/29 7:18, Phillip Lougher wrote:
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@xxxxxxxxx>

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
No more threads than before the patch.

   current, where it is currently restricted to num_online_cpus() * 2.
After the patch is installed, the maximum number of threads is still num_online_cpus() * 2.

[PATCH v2 2/2] squashfs: Allows users to configure the number of decompression threads

+#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


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.
When multiple decompression modes are selected during kernel build, or only SQUASHFS_DECOMP_MULTI is selected during kernel build, the mount parameter "threads=" is meaningful,
However, when only SQUASHFS_DECOMP_SINGLE or SQUASHFS_DECOMP_MULTI_PERCPU is selected, the mount parameter "threads=" is meaningless.

Thank you for your guidance



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.

SQUASHFS_DECOMP_MULTI and SQUASHFS_DECOMP_SINGLE are selected during construction. Thread=1 indicates that SQUASHFS_DECOMP_SINGLEI is used.

If only SQUASHFS_DECOMP_MULTI is selected during construction, thread=1 indicates that SQUASHFS_DECOMP_MULTI is used, and only one decompression thread is created.

Would it be better to provide more flexible mount options for images that build only SQUASHFS_DECOMP_MULTI?


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.
After the patch is installed, the maximum number of threads is still num_online_cpus() * 2.

[PATCH v2 2/2] squashfs: Allows users to configure the number of decompression threads

+#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;

Did I misunderstand your question?



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
Would it be better to allow flexible selection of decompression mode combinations?

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.
In patch 2, the maximum number of threads has been limited,
Have I misunderstood your question



Phillip

---
Phillip Lougher, Squashfs author and maintainer.


Thanks
Xiaoming Ni


v1: https://lore.kernel.org/lkml/20220815031100.75243-1-nixiaoming@xxxxxxxxxx/
----

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




.