Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
IronPort-Data: A9a23:8HCCpaueenIyrvkl59WCIYjOM+fnVNFeMUV32f8akzHdYApBsoF/q
 tZmKW6CO/+LM2v3ft5/OY2z8hsB75HcmoJkTVc/r3s3Qi1HgMeUXt7xwmUckM+xwmwvaGo9s
 q3yv/GZdJhcokf0/0rrav656yAkiclkf5KkYMbcICd9WAR4fykojBNnioYRj5Vh6TSDK1vlV
 eja/YuHYzdJ5xYuajhPsvrY8Es21BjPkGpwUmIWNKgjUGD2zxH5PLpHTYmtIn3xRJVjH+LSb
 44vG5ngows1Vz90Yj+Uuu6Tnn8iG9Y+DiDS4pZiYJVOtzAZzsAEPgnXA9JHAatfo23hc9mcU
 7yhv7ToIesiFvWkdOjwz3C0HgkmVZCq9oMrLlCA8um4n0GbSkHm0s5KPUAtG5Yow75eVDQmG
 fwwcFjhbziKivjzy7WhR6wywM8iN8LseogYvxmMzxmAUq1gGsCFGf2Ro4UCtNszrpgm8fL2b
 s8YYidrZQ/oaBxUMVBRA5U79AutriCnKmQA8gvPzUYxy1n0nClg6KLxCsjQV+fQRMR+rhiBn
 W2TqgwVBTlDZIDAllJp6EmEgu7JgDO+W48IErC83uBljUfVxWEJDhASE1yhrpGRgUmkUd9DA
 0gL/GwooLRa3FemVNbwdx2xpHqVuFgXXN84O+Q58ASlzqvS/hbcCG8ZSDJIdN0hsokxXzNC/
 luImc75QDBou5WLRn+HsLSZtzW/PW4SN2BqTTcETA8B4sHLu5guhwnPQM0lF7S6yMD2cQwc2
 BiDoTJ7hq0Ul9JO0aynu1vGmDmr4JPOS2bZ+zk7QEqarSZSNIL4Xrai+GP/3PdbCp2pdwGo6
 S1sd9el0MgCCpSElSqoSeoLHa206/vtDNE6qQMwd3XG32n9k0NPbby88xkjfhw3Y5ZslSvBP
 RWK41I5CIp7ZiPCUENhX26mI+IOpUQKPf3hU/3OY5J1fpF9HONs1Hg1PBP4M4zFvkwhj6Y7M
 JGXGftA7Er264w6klJapM9Ej9fHIxzSI0uIHPgXKDz9iNKjiIa9E+ttDbd3RrlRAFm4iAvU6
 c1DEMCB1g9SVubzCgGOr99DcwhWdyNqXcivwyCySgJlClc3cI3GI6GJqY7Nh6Q+90iovr6Sp
 yrkChMwJKTX3COZeW1mlUyPmJu2Dcog8hrXzAQnPE2j3DA4cJ2z4aIEH6bbjpF5nNGPOcVcF
 qFfE+3ZW6wnYm2eq1wggWzV8dUKmOKD3lnVYUJIoVEXIvZdeuA+0oa/IFaxpHFQUHHfWAlXi
 +TI6z43iKErH2xKZPs6otr2p79tlRDxQN5PYnY=
IronPort-HdrOrdr: A9a23:k2JGs68EPoC4SKKXMdFuk+D3I+orL9Y04lQ7vn2ZhyY7TiW9rb
 HKoB17726XtN9/YhsdcLy7VZVoBEmskaKdgrNhXotKPjOGhILAFugLhrcKgQeBJ8SUzIBgPM
 lbH5RWOZnZEUV6gcHm4AOxDtoshOWc/LvAv5a485+oJjsaEp2JKGxCe2CmLnE=
Date:   Mon, 30 Oct 2023 18:09:45 +0200
From:   Ariel Miculas <amiculas@cisco.com>
To:     Phillip Lougher <phillip@squashfs.org.uk>
Cc:     linux-doc@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>, serge@hallyn.com,
        Jonathan Corbet <corbet@lwn.net>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] docs: filesystems: document the squashfs specific
 mount options
Message-ID: <oonydtkv6anpjbugjvqkp4byqlf3aiwkwf4g7y5w5tthivxomh@b4w4jyceaepz>
References: <20231029161924.50648-1-amiculas@cisco.com>
 <bd039dc7-cfe5-c210-b65e-0cba3186ee0b@squashfs.org.uk>
 <qgwx4cjswrhzxqijsosix437xyw3ifglv6uz2d3nq7whkjqueo@c5pzoqtoqtgb>
 <46d308b2-5e59-862d-b429-e6ee418bc61e@squashfs.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <46d308b2-5e59-862d-b429-e6ee418bc61e@squashfs.org.uk>
Precedence: bulk

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