[PATCH 0/2] Make squashfs fragments' cache size more configurable

[PATCH 0/2] Make squashfs fragments' cache size more configurable

[Qixuan Wu]

Hi All,

Currently, squashfs fragments' cache size is only determined by
config option CONFIG_SQUASHFS_FRAGMENT_CACHE_SIZE. Users have
no way to change the value when they get the binary kernel.
Now make it be configured when booting or inserting module.
Actually, it's better that a config option in a number format
in .config file cat be reconfigured during booting or inserting
module.

Thanks
Qixuan

Qixuan Wu (2):
  Squashfs: Let the number of fragments cached configurable
  Documentation/kernel-parameters.txt: Add kernel parameter of squashfs
    fragments' cache size

 Documentation/admin-guide/kernel-parameters.txt |  7 ++++
 fs/squashfs/super.c                             | 43 ++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH 1/2] Squashfs: Let the number of fragments cached configurable

[Qixuan Wu]

Currently, squashfs fragments' cache size is only determined by
config option CONFIG_SQUASHFS_FRAGMENT_CACHE_SIZE. Users have
no way to change the value when they get the binary kernel.
Now make it be configured during booting or inserting module.

Signed-off-by: Qixuan Wu <wuqixuan@huawei.com>
Signed-off-by: Chen Jie <chenjie6@huawei.com>
---
 fs/squashfs/super.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
index cf01e15..82f9e46 100644
--- a/fs/squashfs/super.c
+++ b/fs/squashfs/super.c
@@ -38,6 +38,7 @@
 #include <linux/module.h>
 #include <linux/magic.h>
 #include <linux/xattr.h>
+#include <linux/ctype.h>
 
 #include "squashfs_fs.h"
 #include "squashfs_fs_sb.h"
@@ -49,6 +50,37 @@
 static struct file_system_type squashfs_fs_type;
 static const struct super_operations squashfs_super_ops;
 
+static unsigned int squashfs_frags_cache_size = SQUASHFS_CACHED_FRAGMENTS;
+
+#ifndef	MODULE
+static int __init squashfs_frags_cachesize_setup(char *buf)
+{
+	unsigned int arg;
+
+	if (!buf || kstrtouint(buf, 10, &arg) < 0) {
+		pr_err("Bad squashfs fragments' cache size, let it be %d\n",
+			SQUASHFS_CACHED_FRAGMENTS);
+		return 0;
+	}
+
+	if (arg < 3 || arg > 99) {
+		pr_warn("Squashfs fragments' cache size should be in the range "
+			"of [3,99]. Let it be %d\n", SQUASHFS_CACHED_FRAGMENTS);
+		return 0;
+	}
+
+	squashfs_frags_cache_size = arg;
+
+	return 0;
+}
+early_param("squashfs.frags_cache_size", squashfs_frags_cachesize_setup);
+
+#else
+module_param(squashfs_frags_cache_size, uint, 0444);
+MODULE_PARM_DESC(squashfs_frags_cache_size,
+	"Squashfs fragments' cache size, it should be in the range of [3,99]");
+#endif
+
 static const struct squashfs_decompressor *supported_squashfs_filesystem(short
 	major, short minor, short id)
 {
@@ -276,8 +308,17 @@ static int squashfs_fill_super(struct super_block *sb, void *data, int silent)
 	if (fragments == 0)
 		goto check_directory_table;
 
+	if ((squashfs_frags_cache_size < 3)
+		|| (squashfs_frags_cache_size > 99)) {
+		ERROR("Fragments' cache size should be in the range of[3,99]."
+			"Let it be %d\n", SQUASHFS_CACHED_FRAGMENTS);
+		squashfs_frags_cache_size = SQUASHFS_CACHED_FRAGMENTS;
+	}
+
+	TRACE("Fragments' cache size %d\n", squashfs_frags_cache_size);
+
 	msblk->fragment_cache = squashfs_cache_init("fragment",
-		SQUASHFS_CACHED_FRAGMENTS, msblk->block_size);
+		squashfs_frags_cache_size, msblk->block_size);
 	if (msblk->fragment_cache == NULL) {
 		err = -ENOMEM;
 		goto failed_mount;
-- 
2.7.4

[PATCH 2/2] Documentation/kernel-parameters.txt: Add kernel parameter of squashfs fragments' cache size

[Qixuan Wu]

Currently, squashfs fragments' cache size is only determined by
config option CONFIG_SQUASHFS_FRAGMENT_CACHE_SIZE. Users have
no way to change the value when they get the binary kernel.
Now make it be configured during booting.

Signed-off-by: Qixuan Wu <wuqixuan@huawei.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 0549662..67bab3b 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3898,6 +3898,13 @@
 	spia_pedr=
 	spia_peddr=
 
+	squashfs.frags_cache_size=
+			[SQUASHFS] The fragments' cache size. The value should
+			be in the range of [3,99].
+
+			The default value of this parameter is determined by
+			the config option CONFIG_SQUASHFS_FRAGMENT_CACHE_SIZE.
+
 	srcutree.counter_wrap_check [KNL]
 			Specifies how frequently to check for
 			grace-period sequence counter wrap for the
-- 
2.7.4

Re: [PATCH 0/2] Make squashfs fragments' cache size more configurable

[Phillip Lougher]

On Thu, Oct 19, 2017 at 12:50 AM, Qixuan Wu <wuqixuan@huawei.com> wrote:
> Hi All,
>
> Currently, squashfs fragments' cache size is only determined by
> config option CONFIG_SQUASHFS_FRAGMENT_CACHE_SIZE. Users have
> no way to change the value when they get the binary kernel.

Thank-you for the patches, but they're both pointless and dangerous.
Let's be clear here you're trying to change an "expert only" kernel
configuration option into a user changeable option.  This is stupid
because it is not meant for non-experts to change for good reason.

The fragment cache size isn't  some small tweak to the operation of
Squashfs, it fundamentally affects both the performance and memory
overhead of Squashfs.  As such right from its introduction in 2003, it
has been an "expert only" configuration option at build time.  Even
then it is made clear that the default has been carefully chosen, and
it should only be changed in exceptional circumstances.  This
basically means don't change the default unless you really know what
you're doing, and this means tracing of Squashfs against your use-case
to determine caching behaviour.  There is absolutely no other reason
why you'd want to change the default.  This also means it should be
restricted to kernel configuration time only.

Let's be clear again, very few people should ever want to change the
default, and for the "experts" that do want to do so, they can do so
when configuring the kernel.  If you're not in a position to change it
at kernel configuration time then by definition you're not an expert,
and you shouldn't be able to change it anyway and certainly not as a
user.

There is absolutely no use-case here to make this a user changeable
option.  I can see no upsides in doing this, only downsides.

Frankly if you need to change this value at module insert time then
there is something wrong with your system or build process.   If you
want this because you want to build the kernel/modules once, and then
post-facto configure them for various products then it is your build
process that is broken.   If you want this because you want to
dynamically change Squashfs memory usage/caching behaviour post kernel
configuration time it suggests you're trying to adapt Squashfs's
footprint based on available memory.   This is an abuse of the option
as it's only meant to be used after detailed tracing/analysis and
certainly not used to accommodate unforeseen dynamic low memory
situations, and if that's the reason for needing this option, you
should be looking to solve it elsewhere.

Ultimately this has been an "expert"  kernel configuration only option
since its introduction in 2003, and I never been asked to change it,
and I believe this is because people recognise it as such.  I suspect
you're trying to change this for fundamentally bogus reasons.
Moreover Squashfs is used in many different use-cases and
distributions, and I'm not going to make this a user-changeable option
allowing users to insert the Squashfs module in such a way that will
break its performance.

So NACK.

Phillip Lougher (Squashfs maintainer)

> Now make it be configured when booting or inserting module.
> Actually, it's better that a config option in a number format
> in .config file cat be reconfigured during booting or inserting
> module.
>
> Thanks
> Qixuan
>
> Qixuan Wu (2):
>   Squashfs: Let the number of fragments cached configurable
>   Documentation/kernel-parameters.txt: Add kernel parameter of squashfs
>     fragments' cache size
>
>  Documentation/admin-guide/kernel-parameters.txt |  7 ++++
>  fs/squashfs/super.c                             | 43 ++++++++++++++++++++++++-
>  2 files changed, 49 insertions(+), 1 deletion(-)
>
> --
> 2.7.4
>

Re: [PATCH 0/2] Make squashfs fragments' cache size more configurable

[Wuqixuan]

Hi Phillip, 

Thank you for your fast reply. 

On Fri, Oct 20, 2017 at 2:18 PM,  Phillip Lougher ‎<phillip.lougher@gmail.com> wrote:
> On Thu, Oct 19, 2017 at 12:50 AM, Qixuan Wu <wuqixuan@huawei.com> wrote:
>> Hi All,
>>
>> Currently, squashfs fragments' cache size is only determined by
>> config option CONFIG_SQUASHFS_FRAGMENT_CACHE_SIZE. Users have
>> no way to change the value when they get the binary kernel.

> Thank-you for the patches, but they're both pointless and dangerous.
> Let's be clear here you're trying to change an "expert only" kernel
> configuration option into a user changeable option.  This is stupid
> because it is not meant for non-experts to change for good reason.
>
> The fragment cache size isn't  some small tweak to the operation of
> Squashfs, it fundamentally affects both the performance and memory
> overhead of Squashfs.  As such right from its introduction in 2003, it
> has been an "expert only" configuration option at build time.  Even
> then it is made clear that the default has been carefully chosen, and
> it should only be changed in exceptional circumstances.  This
> basically means don't change the default unless you really know what
> you're doing, and this means tracing of Squashfs against your use-case
> to determine caching behaviour.  There is absolutely no other reason
> why you'd want to change the default.  This also means it should be
> restricted to kernel configuration time only.
> 
> Let's be clear again, very few people should ever want to change the
> default, and for the "experts" that do want to do so, they can do so
> when configuring the kernel.  If you're not in a position to change it
> at kernel configuration time then by definition you're not an expert,
> and you shouldn't be able to change it anyway and certainly not as a
> user.
> 
> There is absolutely no use-case here to make this a user changeable
> option.  I can see no upsides in doing this, only downsides.
> 
> Frankly if you need to change this value at module insert time then
> there is something wrong with your system or build process.   If you
> want this because you want to build the kernel/modules once, and then
> post-facto configure them for various products then it is your build
> process that is broken.   If you want this because you want to
> dynamically change Squashfs memory usage/caching behaviour post kernel
> configuration time it suggests you're trying to adapt Squashfs's
> footprint based on available memory.   This is an abuse of the option
> as it's only meant to be used after detailed tracing/analysis and
> certainly not used to accommodate unforeseen dynamic low memory
> situations, and if that's the reason for needing this option, you
> should be looking to solve it elsewhere.
> 
> Ultimately this has been an "expert"  kernel configuration only option
> since its introduction in 2003, and I never been asked to change it,
> and I believe this is because people recognise it as such.  I suspect
> you're trying to change this for fundamentally bogus reasons.
> Moreover Squashfs is used in many different use-cases and
> distributions, and I'm not going to make this a user-changeable option
> allowing users to insert the Squashfs module in such a way that will
> break its performance.
> 
> So NACK.

I need apologize for not describing the scenario clear enough. In our 
company, maybe we have a bit different kernel distribution mode. We 
only can release the single kernel binary to multiple customer. For one
customer of us, they have a very strict kernel boot speed requirement that
is 2~3s including rootfs (squashfs) uncompression. We found if modify the 
value from 3 to 8, some handreds of miliseconds can be saved, and the total 
boot time satisfied the requirement. But we were afraid to impact other 
customer, so used kernel boot parameter. Module interface currently there 
is no user-case. 

Maybe it's not correct, from my opinion, that kernel boot parameter is almost 
same as config option, kernel module, /proc or /sysfs. It gives administrator 
the chance to change some kernel's variables as per different scenario, if 
they cannot chagne the config option. And administrator sometime is root, 
not normal user. So the parameter set by them through kernel boot parameter 
with enough understanding, testing and analysis. For example, such kernel boot
parameters like crashkernel=size[KMG], default_hugepagesz= also do the 
same work. So before setting to 8, the administrator of our customer understands 
the meaning and memory overhead impact of this modification of fragments 
cache size. 

Frankly I admit maybe our scenario is a bit special in embedded system, it's 
not as useful for others as for us. So it seems like a bit over-design and I can 
understand what you are worried about if accepting the feature. 

Anyway, thanks for your reply and your opinion. 

Thanks 
Qixuan

> Phillip Lougher (Squashfs maintainer)