Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
IronPort-Data: A9a23:FWNQXamz71VXIcGJlxmpltLo5gwFJkRdPkR7XQ2eYbSJt1+Wr1Gzt
 xJMUGvVP/3YZDDyKohzO9y/8E1QuJCBn9MwGVNtqylmEltH+JHPbTi7wugcHM8zwunrFh8PA
 xA2M4GYRCwMZiaB4E/rav649SUUOZigHtLUEPTDNj16WThqQSIgjQMLs+Mii+aEu/Dha++2k
 Y20+5a31GONgWYuaTtMsPrb8XuDgdyr0N8mlg1mDRx0lAe2e0k9VPo3Oay3Jn3kdYhYdsbSq
 zHrlezREsvxpn/BO/v9+lrJWhRiro36YWBivkFrt52K2XCukMCdPpETb5LwYW8P49mAcksYJ
 N9l7fRcQi9xVkHAdXh0vxRwS0lD0aN6FLDvcSe1n/KZ/V/9VTjc06h2B0stbYI29bMiaY1O3
 aRwxDElZx2Zwumx2r/+E69nh98oK4/gO4Z3VnNIlG6CS612B8qbGOOQv7e03x9o7ixKNf/bZ
 sEEbTN0RB/BeBZIfFwQDfrSmc/52iOnKGcEwL6TjbcHxGuL8yN36pb8E9PWSuLVZcVrp0nN8
 woq+EygUk1Fa7Rz0wGt9nOqm/+Kni7hXo8WPKO3++Qsg1CJwGEXThoMWjOTqv6jgEOic9ZAL
 QoS9zZGhbM/7kOmZtn0WxmppjiPuRt0c9NTC+gS7ACL17qR7QGEAGQNUj9GbpohrsBebTgr0
 EKZ2t/uDhRxv7CPD3GQ7LGZqXW1Iyd9BXcCZCsATBMt+MD+qZozgAmJScxsVrO25uAZAhn5z
 irPrTA5nalWi8cXka665lvAxTmro/AlUzLZ+C2HAWiL0AlLdrWDJLy4tEDQ5+0QI6iWGwzpU
 Gc/p+CS6+UHDJeonSOLQfkQELzB2xpjGGCD6bKIN8RxnwlB60JPbqgNumogdRcB3tIsJGWwO
 h67VRZ5vsc7AZe8UUNgS6SVYyjA5YHkENn/W7jvctNCCnSaXFbcpXwGiaK48WTgjk8omKczU
 ap3kPpA715HV8yLLxLvGY/xNIPHIAhlnAs/orihlnyaPUK2PiL9dFv8GALmghoFxK2Fuh7J1
 N1UKtGHzR5SOMWnPHiHr9ZPdwtbdSBibXwTlyCxXrPSSuaBMD95Y8I9PZt6E2CYt/0Pz7yRr
 i3VtrFwkQSh1RUr1jlmmlg6OO+wAv6TXFowPDcnOh6zymM/bIO0hJrzhLNpFYTLANdLlKYuJ
 9FcIp3oKq0WGlz6F8E1MMCVQHpKL0/w22pj/kONPVACQnKXb1WSpIK6LlOxqnJm4+jenZJWn
 oBMHzjzGfIrLzmOxu6PAB5z5ztdZUQgpd8=
IronPort-HdrOrdr: A9a23:p+L1LaGEYUccSKitpLqEx8eALOsnbusQ8zAXPo5KJiC9Vvbo8v
 xG/c5rtyMc5wx6ZJhNo7290ey7MBfhHP1OkOos1NWZPTUO0VHAROpfBMnZowEIcBeRygcy78
 tdWpk7IMHsDFR8kMbx6BS1HpId2tWdmZrY4ts3CxxWPHhXg2YK1XYeNjqm
Date:   Mon, 30 Oct 2023 18:42:32 +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: <nlpgfo5g4jzbb5hc2wu5a3mlnipgbjki7fqcgwe5jknxjmgbwa@2tpkg35xsuy4>
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>
 <oonydtkv6anpjbugjvqkp4byqlf3aiwkwf4g7y5w5tthivxomh@b4w4jyceaepz>
 <332ff885-f621-883a-696a-e8247fb43ecc@squashfs.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <332ff885-f621-883a-696a-e8247fb43ecc@squashfs.org.uk>
Precedence: bulk

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