* [PATCH v2] docs: filesystems: document the squashfs specific mount options @ 2023-10-29 16:19 Ariel Miculas 2023-10-29 20:33 ` Phillip Lougher 2023-10-30 1:00 ` Bagas Sanjaya 0 siblings, 2 replies; 8+ messages in thread From: Ariel Miculas @ 2023-10-29 16:19 UTC (permalink / raw) To: linux-doc Cc: serge, Ariel Miculas, Phillip Lougher, Jonathan Corbet, linux-kernel When SQUASHFS_CHOICE_DECOMP_BY_MOUNT is set, the "threads" mount option can be used to specify the decompression mode: single-threaded, multi-threaded, percpu or the number of threads used for decompression. When SQUASHFS_CHOICE_DECOMP_BY_MOUNT is not set and SQUASHFS_DECOMP_MULTI is set, the "threads" option can also be used to specify the number of threads used for decompression. This mount option is only mentioned in fs/squashfs/Kconfig, which makes it difficult to find. Another mount option available is "errors", which can be configured to panic the kernel when squashfs errors are encountered. Add both these options to the squashfs documentation, making them more noticeable. Signed-off-by: Ariel Miculas <amiculas@cisco.com> --- Documentation/filesystems/squashfs.rst | 59 ++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/Documentation/filesystems/squashfs.rst b/Documentation/filesystems/squashfs.rst index df42106bae71..0a7fa66b70f8 100644 --- a/Documentation/filesystems/squashfs.rst +++ b/Documentation/filesystems/squashfs.rst @@ -64,6 +64,65 @@ obtained from this site also. The squashfs-tools development tree is now located on kernel.org git://git.kernel.org/pub/scm/fs/squashfs/squashfs-tools.git +2.1 Mount options +----------------- +=================== ========================================================= +errors=%s Specify whether squashfs errors trigger a kernel panic + or not + + ========== ============================================= + continue errors don't trigger a panic (default) + panic trigger a panic when errors are encountered, + similar to several other filesystems (e.g. + btrfs, ext4, f2fs, GFS2, jfs, ntfs, ubifs) + + This allows a kernel dump to be saved, + useful for analyzing and debugging the + corruption. + ========== ============================================= +threads=%s Select the decompression mode or the number of threads + + If SQUASHFS_CHOICE_DECOMP_BY_MOUNT is set: + + ========== ============================================= + single use single-threaded decompression (default) + + Only one block (data or metadata) can be + decompressed at any one time. This limits + CPU and memory usage to a minimum, but it + also gives poor performance on parallel I/O + workloads when using multiple CPU machines + due to waiting on decompressor availability. + multi use up to two parallel decompressors per core + + If you have a parallel I/O workload and your + system has enough memory, using this option + may improve overall I/O performance. It + dynamically allocates decompressors on a + demand basis. + percpu use a maximum of one decompressor per core + + It uses percpu variables to ensure + decompression is load-balanced across the + cores. + 1|2|3|... configure the number of threads used for + decompression + + The upper limit is num_online_cpus() * 2. + ========== ============================================= + + If SQUASHFS_CHOICE_DECOMP_BY_MOUNT is **not** set and + SQUASHFS_DECOMP_MULTI is set: + + ========== ============================================= + 2|3|... configure the number of threads used for + decompression + + The upper limit is num_online_cpus() * 2. + ========== ============================================= + +=================== ========================================================= + 3. Squashfs Filesystem Design ----------------------------- -- 2.42.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] docs: filesystems: document the squashfs specific mount options 2023-10-29 16:19 [PATCH v2] docs: filesystems: document the squashfs specific mount options Ariel Miculas @ 2023-10-29 20:33 ` Phillip Lougher 2023-10-30 12:57 ` Ariel Miculas 2023-10-30 1:00 ` Bagas Sanjaya 1 sibling, 1 reply; 8+ messages in thread From: Phillip Lougher @ 2023-10-29 20:33 UTC (permalink / raw) To: Ariel Miculas, linux-doc, Andrew Morton Cc: serge, Jonathan Corbet, linux-kernel On 29/10/2023 16:19, Ariel Miculas wrote: > When SQUASHFS_CHOICE_DECOMP_BY_MOUNT is set, the "threads" mount option > can be used to specify the decompression mode: single-threaded, > multi-threaded, percpu or the number of threads used for decompression. > When SQUASHFS_CHOICE_DECOMP_BY_MOUNT is not set and > SQUASHFS_DECOMP_MULTI is set, the "threads" option can also be used to > specify the number of threads used for decompression. This mount option > is only mentioned in fs/squashfs/Kconfig, which makes it difficult to > find. > > Another mount option available is "errors", which can be configured to > panic the kernel when squashfs errors are encountered. > > Add both these options to the squashfs documentation, making them more > noticeable. > > Signed-off-by: Ariel Miculas <amiculas@cisco.com> Looks good to me. Reviewed-by: Phillip Lougher <phillip@squashfs.org.uk> > --- > Documentation/filesystems/squashfs.rst | 59 ++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/Documentation/filesystems/squashfs.rst b/Documentation/filesystems/squashfs.rst > index df42106bae71..0a7fa66b70f8 100644 > --- a/Documentation/filesystems/squashfs.rst > +++ b/Documentation/filesystems/squashfs.rst > @@ -64,6 +64,65 @@ obtained from this site also. > The squashfs-tools development tree is now located on kernel.org > git://git.kernel.org/pub/scm/fs/squashfs/squashfs-tools.git > > +2.1 Mount options > +----------------- > +=================== ========================================================= > +errors=%s Specify whether squashfs errors trigger a kernel panic > + or not > + > + ========== ============================================= > + continue errors don't trigger a panic (default) > + panic trigger a panic when errors are encountered, > + similar to several other filesystems (e.g. > + btrfs, ext4, f2fs, GFS2, jfs, ntfs, ubifs) > + > + This allows a kernel dump to be saved, > + useful for analyzing and debugging the > + corruption. > + ========== ============================================= > +threads=%s Select the decompression mode or the number of threads > + > + If SQUASHFS_CHOICE_DECOMP_BY_MOUNT is set: > + > + ========== ============================================= > + single use single-threaded decompression (default) > + > + Only one block (data or metadata) can be > + decompressed at any one time. This limits > + CPU and memory usage to a minimum, but it > + also gives poor performance on parallel I/O > + workloads when using multiple CPU machines > + due to waiting on decompressor availability. > + multi use up to two parallel decompressors per core > + > + If you have a parallel I/O workload and your > + system has enough memory, using this option > + may improve overall I/O performance. It > + dynamically allocates decompressors on a > + demand basis. > + percpu use a maximum of one decompressor per core > + > + It uses percpu variables to ensure > + decompression is load-balanced across the > + cores. > + 1|2|3|... configure the number of threads used for > + decompression > + > + The upper limit is num_online_cpus() * 2. > + ========== ============================================= > + > + If SQUASHFS_CHOICE_DECOMP_BY_MOUNT is **not** set and > + SQUASHFS_DECOMP_MULTI is set: > + > + ========== ============================================= > + 2|3|... configure the number of threads used for > + decompression > + > + The upper limit is num_online_cpus() * 2. > + ========== ============================================= > + > +=================== ========================================================= > + > 3. Squashfs Filesystem Design > ----------------------------- > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] docs: filesystems: document the squashfs specific mount options 2023-10-29 20:33 ` Phillip Lougher @ 2023-10-30 12:57 ` Ariel Miculas 2023-10-30 15:40 ` Phillip Lougher 0 siblings, 1 reply; 8+ messages in thread From: Ariel Miculas @ 2023-10-30 12:57 UTC (permalink / raw) To: Phillip Lougher Cc: linux-doc, Andrew Morton, serge, Jonathan Corbet, linux-kernel On 23/10/29 08:33PM, Phillip Lougher wrote: > On 29/10/2023 16:19, Ariel Miculas wrote: > > When SQUASHFS_CHOICE_DECOMP_BY_MOUNT is set, the "threads" mount option > > can be used to specify the decompression mode: single-threaded, > > multi-threaded, percpu or the number of threads used for decompression. > > When SQUASHFS_CHOICE_DECOMP_BY_MOUNT is not set and > > SQUASHFS_DECOMP_MULTI is set, the "threads" option can also be used to > > specify the number of threads used for decompression. This mount option > > is only mentioned in fs/squashfs/Kconfig, which makes it difficult to > > find. > > > > Another mount option available is "errors", which can be configured to > > panic the kernel when squashfs errors are encountered. > > > > Add both these options to the squashfs documentation, making them more > > noticeable. > > > > Signed-off-by: Ariel Miculas <amiculas@cisco.com> > > Looks good to me. > > Reviewed-by: Phillip Lougher <phillip@squashfs.org.uk> Unfortunately, it seems this is not quite correct either: There is the config option SQUASHFS_MOUNT_DECOMP_THREADS: ``` bool "Add the mount parameter 'threads=' for squashfs" depends on SQUASHFS depends on SQUASHFS_DECOMP_MULTI default n help Use threads= to set the decompression parallel mode and the number of threads. If SQUASHFS_CHOICE_DECOMP_BY_MOUNT=y threads=<single|multi|percpu|1|2|3|...> else threads=<2|3|...> The upper limit is num_online_cpus() * 2. ``` that depends on SQUASHFS_DECOMP_MULTI. So I think I should take my v1 patch and specify that the "threads=" mount option depends on SQUASHFS_MOUNT_DECOMP_THREADS. There's no need to specify SQUASHFS_DECOMP_MULTI, because SQUASHFS_MOUNT_DECOMP_THREADS already depends on it. What do you think? Regards, Ariel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] docs: filesystems: document the squashfs specific mount options 2023-10-30 12:57 ` Ariel Miculas @ 2023-10-30 15:40 ` Phillip Lougher 2023-10-30 16:09 ` Ariel Miculas 0 siblings, 1 reply; 8+ messages in thread From: Phillip Lougher @ 2023-10-30 15:40 UTC (permalink / raw) To: Ariel Miculas Cc: linux-doc, Andrew Morton, serge, Jonathan Corbet, linux-kernel On 30/10/2023 12:57, Ariel Miculas wrote: > On 23/10/29 08:33PM, Phillip Lougher wrote: >> On 29/10/2023 16:19, Ariel Miculas wrote: >>> When SQUASHFS_CHOICE_DECOMP_BY_MOUNT is set, the "threads" mount option >>> can be used to specify the decompression mode: single-threaded, >>> multi-threaded, percpu or the number of threads used for decompression. >>> When SQUASHFS_CHOICE_DECOMP_BY_MOUNT is not set and >>> SQUASHFS_DECOMP_MULTI is set, the "threads" option can also be used to >>> specify the number of threads used for decompression. This mount option >>> is only mentioned in fs/squashfs/Kconfig, which makes it difficult to >>> find. >>> >>> Another mount option available is "errors", which can be configured to >>> panic the kernel when squashfs errors are encountered. >>> >>> Add both these options to the squashfs documentation, making them more >>> noticeable. >>> >>> Signed-off-by: Ariel Miculas <amiculas@cisco.com> >> >> Looks good to me. >> >> Reviewed-by: Phillip Lougher <phillip@squashfs.org.uk> > > Unfortunately, it seems this is not quite correct either: > There is the config option SQUASHFS_MOUNT_DECOMP_THREADS: > ``` > bool "Add the mount parameter 'threads=' for squashfs" > depends on SQUASHFS > depends on SQUASHFS_DECOMP_MULTI > default n > help > Use threads= to set the decompression parallel mode and the number of threads. > If SQUASHFS_CHOICE_DECOMP_BY_MOUNT=y > threads=<single|multi|percpu|1|2|3|...> > else > threads=<2|3|...> > The upper limit is num_online_cpus() * 2. > ``` > that depends on SQUASHFS_DECOMP_MULTI. > So I think I should take my v1 patch and specify that the "threads=" > mount option depends on SQUASHFS_MOUNT_DECOMP_THREADS. There's no need > to specify SQUASHFS_DECOMP_MULTI, because SQUASHFS_MOUNT_DECOMP_THREADS > already depends on it. Sorry, you have to specify SQUASHFS_DECOMP_MULTI to be able to specify SQUASHFS_MOUNT_DECOMP_THREADS if SQUASHFS_DECOMP_BY_MOUNT is unselected. Just try it, do make menuconfig, ensure SQUASHFS_CHOICE_DECOMP_BY_MOUNT is unselected, select Single threaded decompression and you won't be able to specify SQUASHFS_MOUNT_DECOMP_THREADS. That was the point of my review. What bit don't you understand? Phillip > What do you think? > > Regards, > Ariel > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] docs: filesystems: document the squashfs specific mount options 2023-10-30 15:40 ` Phillip Lougher @ 2023-10-30 16:09 ` Ariel Miculas 2023-10-30 16:28 ` Phillip Lougher 0 siblings, 1 reply; 8+ messages in thread From: Ariel Miculas @ 2023-10-30 16:09 UTC (permalink / raw) To: Phillip Lougher Cc: linux-doc, Andrew Morton, serge, Jonathan Corbet, linux-kernel On 23/10/30 03:40PM, Phillip Lougher wrote: > On 30/10/2023 12:57, Ariel Miculas wrote: > > On 23/10/29 08:33PM, Phillip Lougher wrote: > > > On 29/10/2023 16:19, Ariel Miculas wrote: > > > > When SQUASHFS_CHOICE_DECOMP_BY_MOUNT is set, the "threads" mount option > > > > can be used to specify the decompression mode: single-threaded, > > > > multi-threaded, percpu or the number of threads used for decompression. > > > > When SQUASHFS_CHOICE_DECOMP_BY_MOUNT is not set and > > > > SQUASHFS_DECOMP_MULTI is set, the "threads" option can also be used to > > > > specify the number of threads used for decompression. This mount option > > > > is only mentioned in fs/squashfs/Kconfig, which makes it difficult to > > > > find. > > > > > > > > Another mount option available is "errors", which can be configured to > > > > panic the kernel when squashfs errors are encountered. > > > > > > > > Add both these options to the squashfs documentation, making them more > > > > noticeable. > > > > > > > > Signed-off-by: Ariel Miculas <amiculas@cisco.com> > > > > > > Looks good to me. > > > > > > Reviewed-by: Phillip Lougher <phillip@squashfs.org.uk> > > > > Unfortunately, it seems this is not quite correct either: > > There is the config option SQUASHFS_MOUNT_DECOMP_THREADS: > > ``` > > bool "Add the mount parameter 'threads=' for squashfs" > > depends on SQUASHFS > > depends on SQUASHFS_DECOMP_MULTI > > default n > > help > > Use threads= to set the decompression parallel mode and the number of threads. > > If SQUASHFS_CHOICE_DECOMP_BY_MOUNT=y > > threads=<single|multi|percpu|1|2|3|...> > > else > > threads=<2|3|...> > > The upper limit is num_online_cpus() * 2. > > ``` > > that depends on SQUASHFS_DECOMP_MULTI. > > So I think I should take my v1 patch and specify that the "threads=" > > mount option depends on SQUASHFS_MOUNT_DECOMP_THREADS. There's no need > > to specify SQUASHFS_DECOMP_MULTI, because SQUASHFS_MOUNT_DECOMP_THREADS > > already depends on it. > > Sorry, you have to specify SQUASHFS_DECOMP_MULTI to be able to specify > SQUASHFS_MOUNT_DECOMP_THREADS if SQUASHFS_DECOMP_BY_MOUNT is unselected. Agree. > > Just try it, do make menuconfig, ensure SQUASHFS_CHOICE_DECOMP_BY_MOUNT > is unselected, select Single threaded decompression and you won't be > able to specify SQUASHFS_MOUNT_DECOMP_THREADS. True. > > That was the point of my review. What bit don't you understand? But SQUASHFS_DECOMP_MULTI is not enough, you need to specify SQUASHFS_MOUNT_DECOMP_THREADS in order to use the "threads=" mount option. So instead of saying ``` If SQUASHFS_CHOICE_DECOMP_BY_MOUNT is **not** set and SQUASHFS_DECOMP_MULTI is set: ``` wouldn't it be right to actually say: ``` If SQUASHFS_CHOICE_DECOMP_BY_MOUNT is **not** set and SQUASHFS_MOUNT_DECOMP_THREADS is set: ```? As you've mentioned, you could only set SQUASHFS_MOUNT_DECOMP_THREADS when SQUASHFS_DECOMP_MULTI is selected. That happens in two cases: * either SQUASHFS_CHOICE_DECOMP_BY_MOUNT is set, in which case it also selects SQUASHFS_MOUNT_DECOMP_THREADS * either SQUASHFS_CHOICE_DECOMP_BY_MOUNT is not set, SQUASHFS_DECOMP_MULTI is set and SQUASHFS_MOUNT_DECOMP_THREADS is also set So I wouldn't even mention SQUASHFS_DECOMP_MULTI in the documentation, only SQUASHFS_MOUNT_DECOMP_THREADS, because the latter always depends on the former. And the "threads=" mount option is only available when SQUASHFS_MOUNT_DECOMP_THREADS is set (which is the configuration I've missed in v1 and v2). Regards, Ariel > > Phillip > > > What do you think? > > > > Regards, > > Ariel > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] docs: filesystems: document the squashfs specific mount options 2023-10-30 16:09 ` Ariel Miculas @ 2023-10-30 16:28 ` Phillip Lougher 2023-10-30 16:42 ` Ariel Miculas 0 siblings, 1 reply; 8+ messages in thread From: Phillip Lougher @ 2023-10-30 16:28 UTC (permalink / raw) To: Ariel Miculas Cc: linux-doc, Andrew Morton, serge, Jonathan Corbet, linux-kernel On 30/10/2023 16:09, Ariel Miculas wrote: > On 23/10/30 03:40PM, Phillip Lougher wrote: >> On 30/10/2023 12:57, Ariel Miculas wrote: >>> On 23/10/29 08:33PM, Phillip Lougher wrote: >>>> On 29/10/2023 16:19, Ariel Miculas wrote: >>>>> When SQUASHFS_CHOICE_DECOMP_BY_MOUNT is set, the "threads" mount option >>>>> can be used to specify the decompression mode: single-threaded, >>>>> multi-threaded, percpu or the number of threads used for decompression. >>>>> When SQUASHFS_CHOICE_DECOMP_BY_MOUNT is not set and >>>>> SQUASHFS_DECOMP_MULTI is set, the "threads" option can also be used to >>>>> specify the number of threads used for decompression. This mount option >>>>> is only mentioned in fs/squashfs/Kconfig, which makes it difficult to >>>>> find. >>>>> >>>>> Another mount option available is "errors", which can be configured to >>>>> panic the kernel when squashfs errors are encountered. >>>>> >>>>> Add both these options to the squashfs documentation, making them more >>>>> noticeable. >>>>> >>>>> Signed-off-by: Ariel Miculas <amiculas@cisco.com> >>>> >>>> Looks good to me. >>>> >>>> Reviewed-by: Phillip Lougher <phillip@squashfs.org.uk> >>> >>> Unfortunately, it seems this is not quite correct either: >>> There is the config option SQUASHFS_MOUNT_DECOMP_THREADS: >>> ``` >>> bool "Add the mount parameter 'threads=' for squashfs" >>> depends on SQUASHFS >>> depends on SQUASHFS_DECOMP_MULTI >>> default n >>> help >>> Use threads= to set the decompression parallel mode and the number of threads. >>> If SQUASHFS_CHOICE_DECOMP_BY_MOUNT=y >>> threads=<single|multi|percpu|1|2|3|...> >>> else >>> threads=<2|3|...> >>> The upper limit is num_online_cpus() * 2. >>> ``` >>> that depends on SQUASHFS_DECOMP_MULTI. >>> So I think I should take my v1 patch and specify that the "threads=" >>> mount option depends on SQUASHFS_MOUNT_DECOMP_THREADS. There's no need >>> to specify SQUASHFS_DECOMP_MULTI, because SQUASHFS_MOUNT_DECOMP_THREADS >>> already depends on it. >> >> Sorry, you have to specify SQUASHFS_DECOMP_MULTI to be able to specify >> SQUASHFS_MOUNT_DECOMP_THREADS if SQUASHFS_DECOMP_BY_MOUNT is unselected. > Agree. >> >> Just try it, do make menuconfig, ensure SQUASHFS_CHOICE_DECOMP_BY_MOUNT >> is unselected, select Single threaded decompression and you won't be >> able to specify SQUASHFS_MOUNT_DECOMP_THREADS. > True. >> >> That was the point of my review. What bit don't you understand? > But SQUASHFS_DECOMP_MULTI is not enough, you need to specify > SQUASHFS_MOUNT_DECOMP_THREADS in order to use the "threads=" mount > option. > So instead of saying > ``` > If SQUASHFS_CHOICE_DECOMP_BY_MOUNT is **not** set and > SQUASHFS_DECOMP_MULTI is set: > ``` > wouldn't it be right to actually say: > ``` > If SQUASHFS_CHOICE_DECOMP_BY_MOUNT is **not** set and > SQUASHFS_MOUNT_DECOMP_THREADS is set: > ```? > > As you've mentioned, you could only set SQUASHFS_MOUNT_DECOMP_THREADS > when SQUASHFS_DECOMP_MULTI is selected. That happens in two cases: > * either SQUASHFS_CHOICE_DECOMP_BY_MOUNT is set, in which case it also > selects SQUASHFS_MOUNT_DECOMP_THREADS > * either SQUASHFS_CHOICE_DECOMP_BY_MOUNT is not set, > SQUASHFS_DECOMP_MULTI is set and SQUASHFS_MOUNT_DECOMP_THREADS is also > set > > So I wouldn't even mention SQUASHFS_DECOMP_MULTI in the documentation, > only SQUASHFS_MOUNT_DECOMP_THREADS, because the latter always depends on > the former. And the "threads=" mount option is only available when > SQUASHFS_MOUNT_DECOMP_THREADS is set (which is the configuration I've > missed in v1 and v2). > You seem determined to create an unpleasant argument here by trying to argue your first patch was correct, and I, as merely the maintainer, can't tell you to change it to how I want it. If, as you pointed out in your first patch, the purpose is to document the mount options, then the fact that threads=xxx option depends on SQUASHFS_DECOMP_MULTI being selected is important, and should be mentioned. I have accepted your V2. If you want to withdraw it now, please do so. But I will not accept further patches from you, nor respond to any more of your emails. It is never a good look to argue with the maintainer, when I am trying to help you make your patch better. You attitude means this has been a waste of my time so far. Phillip > Regards, > Ariel >> >> Phillip >> >>> What do you think? >>> >>> Regards, >>> Ariel >>> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] docs: filesystems: document the squashfs specific mount options 2023-10-30 16:28 ` Phillip Lougher @ 2023-10-30 16:42 ` Ariel Miculas 0 siblings, 0 replies; 8+ messages in thread From: Ariel Miculas @ 2023-10-30 16:42 UTC (permalink / raw) To: Phillip Lougher Cc: linux-doc, Andrew Morton, serge, Jonathan Corbet, linux-kernel On 23/10/30 04:28PM, Phillip Lougher wrote: > On 30/10/2023 16:09, Ariel Miculas wrote: > > On 23/10/30 03:40PM, Phillip Lougher wrote: > > > On 30/10/2023 12:57, Ariel Miculas wrote: > > > > On 23/10/29 08:33PM, Phillip Lougher wrote: > > > > > On 29/10/2023 16:19, Ariel Miculas wrote: > > > > > > When SQUASHFS_CHOICE_DECOMP_BY_MOUNT is set, the "threads" mount option > > > > > > can be used to specify the decompression mode: single-threaded, > > > > > > multi-threaded, percpu or the number of threads used for decompression. > > > > > > When SQUASHFS_CHOICE_DECOMP_BY_MOUNT is not set and > > > > > > SQUASHFS_DECOMP_MULTI is set, the "threads" option can also be used to > > > > > > specify the number of threads used for decompression. This mount option > > > > > > is only mentioned in fs/squashfs/Kconfig, which makes it difficult to > > > > > > find. > > > > > > > > > > > > Another mount option available is "errors", which can be configured to > > > > > > panic the kernel when squashfs errors are encountered. > > > > > > > > > > > > Add both these options to the squashfs documentation, making them more > > > > > > noticeable. > > > > > > > > > > > > Signed-off-by: Ariel Miculas <amiculas@cisco.com> > > > > > > > > > > Looks good to me. > > > > > > > > > > Reviewed-by: Phillip Lougher <phillip@squashfs.org.uk> > > > > > > > > Unfortunately, it seems this is not quite correct either: > > > > There is the config option SQUASHFS_MOUNT_DECOMP_THREADS: > > > > ``` > > > > bool "Add the mount parameter 'threads=' for squashfs" > > > > depends on SQUASHFS > > > > depends on SQUASHFS_DECOMP_MULTI > > > > default n > > > > help > > > > Use threads= to set the decompression parallel mode and the number of threads. > > > > If SQUASHFS_CHOICE_DECOMP_BY_MOUNT=y > > > > threads=<single|multi|percpu|1|2|3|...> > > > > else > > > > threads=<2|3|...> > > > > The upper limit is num_online_cpus() * 2. > > > > ``` > > > > that depends on SQUASHFS_DECOMP_MULTI. > > > > So I think I should take my v1 patch and specify that the "threads=" > > > > mount option depends on SQUASHFS_MOUNT_DECOMP_THREADS. There's no need > > > > to specify SQUASHFS_DECOMP_MULTI, because SQUASHFS_MOUNT_DECOMP_THREADS > > > > already depends on it. > > > > > > Sorry, you have to specify SQUASHFS_DECOMP_MULTI to be able to specify > > > SQUASHFS_MOUNT_DECOMP_THREADS if SQUASHFS_DECOMP_BY_MOUNT is unselected. > > Agree. > > > > > > Just try it, do make menuconfig, ensure SQUASHFS_CHOICE_DECOMP_BY_MOUNT > > > is unselected, select Single threaded decompression and you won't be > > > able to specify SQUASHFS_MOUNT_DECOMP_THREADS. > > True. > > > > > > That was the point of my review. What bit don't you understand? > > But SQUASHFS_DECOMP_MULTI is not enough, you need to specify > > SQUASHFS_MOUNT_DECOMP_THREADS in order to use the "threads=" mount > > option. > > So instead of saying > > ``` > > If SQUASHFS_CHOICE_DECOMP_BY_MOUNT is **not** set and > > SQUASHFS_DECOMP_MULTI is set: > > ``` > > wouldn't it be right to actually say: > > ``` > > If SQUASHFS_CHOICE_DECOMP_BY_MOUNT is **not** set and > > SQUASHFS_MOUNT_DECOMP_THREADS is set: > > ```? > > > > As you've mentioned, you could only set SQUASHFS_MOUNT_DECOMP_THREADS > > when SQUASHFS_DECOMP_MULTI is selected. That happens in two cases: > > * either SQUASHFS_CHOICE_DECOMP_BY_MOUNT is set, in which case it also > > selects SQUASHFS_MOUNT_DECOMP_THREADS > > * either SQUASHFS_CHOICE_DECOMP_BY_MOUNT is not set, > > SQUASHFS_DECOMP_MULTI is set and SQUASHFS_MOUNT_DECOMP_THREADS is also > > set > > > > So I wouldn't even mention SQUASHFS_DECOMP_MULTI in the documentation, > > only SQUASHFS_MOUNT_DECOMP_THREADS, because the latter always depends on > > the former. And the "threads=" mount option is only available when > > SQUASHFS_MOUNT_DECOMP_THREADS is set (which is the configuration I've > > missed in v1 and v2). > > > > You seem determined to create an unpleasant argument here by trying to argue > your first patch was correct, and I, as merely the maintainer, can't tell > you to change it to how I want it. I'm not trying to argue that my first patch is correct, I'm sorry if it seems this way. I'm trying to argue that my v2 patch is also missing the mention of SQUASHFS_MOUNT_DECOMP_THREADS. > > If, as you pointed out in your first patch, the purpose is to > document the mount options, then the fact that threads=xxx > option depends on SQUASHFS_DECOMP_MULTI being selected is > important, and should be mentioned. Ok, I understand this. I also think it's important to mention SQUASHFS_MOUNT_DECOMP_THREADS, which was an oversight from my part. > > I have accepted your V2. If you want to withdraw it now, please > do so. But I will not accept further patches from you, nor > respond to any more of your emails. > > It is never a good look to argue with the maintainer, when I am > trying to help you make your patch better. You attitude means this > has been a waste of my time so far. I didn't want to argue against your suggestion, I did appreciate it and I have sent a v2 patch implementing it. I only wanted to point out a missing configuration option that, in my opinion, should also be part of the documentation. If you think that's not the case, let me know. Regards, Ariel > > Phillip > > > Regards, > > Ariel > > > > > > Phillip > > > > > > > What do you think? > > > > > > > > Regards, > > > > Ariel > > > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] docs: filesystems: document the squashfs specific mount options 2023-10-29 16:19 [PATCH v2] docs: filesystems: document the squashfs specific mount options Ariel Miculas 2023-10-29 20:33 ` Phillip Lougher @ 2023-10-30 1:00 ` Bagas Sanjaya 1 sibling, 0 replies; 8+ messages in thread From: Bagas Sanjaya @ 2023-10-30 1:00 UTC (permalink / raw) To: Ariel Miculas, linux-doc Cc: serge, Phillip Lougher, Jonathan Corbet, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3643 bytes --] On Sun, Oct 29, 2023 at 06:19:24PM +0200, Ariel Miculas wrote: > +2.1 Mount options > +----------------- > +=================== ========================================================= > +errors=%s Specify whether squashfs errors trigger a kernel panic > + or not > + > + ========== ============================================= > + continue errors don't trigger a panic (default) > + panic trigger a panic when errors are encountered, > + similar to several other filesystems (e.g. > + btrfs, ext4, f2fs, GFS2, jfs, ntfs, ubifs) > + > + This allows a kernel dump to be saved, > + useful for analyzing and debugging the > + corruption. > + ========== ============================================= > +threads=%s Select the decompression mode or the number of threads > + > + If SQUASHFS_CHOICE_DECOMP_BY_MOUNT is set: > + > + ========== ============================================= > + single use single-threaded decompression (default) > + > + Only one block (data or metadata) can be > + decompressed at any one time. This limits > + CPU and memory usage to a minimum, but it > + also gives poor performance on parallel I/O > + workloads when using multiple CPU machines > + due to waiting on decompressor availability. > + multi use up to two parallel decompressors per core > + > + If you have a parallel I/O workload and your > + system has enough memory, using this option > + may improve overall I/O performance. It > + dynamically allocates decompressors on a > + demand basis. > + percpu use a maximum of one decompressor per core > + > + It uses percpu variables to ensure > + decompression is load-balanced across the > + cores. > + 1|2|3|... configure the number of threads used for > + decompression > + > + The upper limit is num_online_cpus() * 2. > + ========== ============================================= > + > + If SQUASHFS_CHOICE_DECOMP_BY_MOUNT is **not** set and > + SQUASHFS_DECOMP_MULTI is set: > + > + ========== ============================================= > + 2|3|... configure the number of threads used for > + decompression > + > + The upper limit is num_online_cpus() * 2. > + ========== ============================================= > + > +=================== ========================================================= > + > The doc LGTM, thanks! Reviewed-by: Bagas Sanjaya <bagasdotme@gmail.com> -- An old man doll... just what I always wanted! - Clara [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-10-30 16:42 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-29 16:19 [PATCH v2] docs: filesystems: document the squashfs specific mount options Ariel Miculas 2023-10-29 20:33 ` Phillip Lougher 2023-10-30 12:57 ` Ariel Miculas 2023-10-30 15:40 ` Phillip Lougher 2023-10-30 16:09 ` Ariel Miculas 2023-10-30 16:28 ` Phillip Lougher 2023-10-30 16:42 ` Ariel Miculas 2023-10-30 1:00 ` Bagas Sanjaya
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).