Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <9596fa83-f06c-5582-7f99-c99c67e73dba@squashfs.org.uk>
Date:   Sun, 29 Oct 2023 12:51:59 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH] docs: filesystems: document the squashfs specific mount
 options
To:     Ariel Miculas <amiculas@cisco.com>, linux-doc@vger.kernel.org
Cc:     serge@hallyn.com, Jonathan Corbet <corbet@lwn.net>,
        linux-kernel@vger.kernel.org
References: <20231027150814.217689-1-amiculas@cisco.com>
From:   Phillip Lougher <phillip@squashfs.org.uk>
In-Reply-To: <20231027150814.217689-1-amiculas@cisco.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 27/10/2023 16:08, 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 or percpu. It can also be used to specify the number of
> threads used for decompression.
> This 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
Good idea to add the options to the Squashfs documentation.

Unfortunately some of the information in the patch is incorrect ... I've
commented below where that is.

So NAK this patch.  Please correct and send a V2 patch.

Thanks

Phillip

> ---
>   Documentation/filesystems/squashfs.rst | 58 ++++++++++++++++++++++++++
>   1 file changed, 58 insertions(+)
> 
> diff --git a/Documentation/filesystems/squashfs.rst b/Documentation/filesystems/squashfs.rst
> index df42106bae71..f71ac870603b 100644
> --- a/Documentation/filesystems/squashfs.rst
> +++ b/Documentation/filesystems/squashfs.rst
> @@ -64,6 +64,64 @@ 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:

In this case it also depends on the SQUASHFS_DECOMP_MULTI decompressor
having been selected.  In otherwords threads=xxx won't work if the
single threaded (SQUASHFS_DECOMP_SINGLE) or percpu threaded
(SQUASHFS_DECOMP_MULTI_PERCPU) decompressors have been selected at build
time.

The reason for this is quite simple, due to their implementation the
number of threads they use is fixed and cannot be changed.

Changing the line to something like

If SQUASHFS_CHOICE_DECOMP_BY_MOUNT is **not** set and
SQUASHFS_DECOMP_MULTI is set

should be OK here.