Messages in this thread | | | Subject | Re: ping //Re: [PATCH v2 0/2] squashfs: Add the mount parameter "threads=" | From | Xiaoming Ni <> | Date | Tue, 30 Aug 2022 21:38:28 +0800 |
| |
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@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 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@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(-) >>> >> > > > .
| |