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

[Phillip Lougher]

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