Date:   Sat, 20 May 2017 19:48:09 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     klondike <klondike@...ndike.es>, linux-kernel@...r.kernel.org,
        npiggin@...il.com
Cc:     P J P <ppandit@...hat.com>, Paul Bolle <pebolle@...cali.nl>,
        Andrew Morton <akpm@...ux-foundation.org>, mmarek@...e.cz
Subject: Re: [v3, 2/2] initramfs: Allow again choice of the embedded initram
 compression algorithm



On 05/20/2017 03:30 PM, Florian Fainelli wrote:
> Hi Francisco, Nicholas,
> 
> Nicholas already fixed part of this commit, but there is more breakage,
> see below:
> 
> On 09/27/2016 01:32 PM, klondike wrote:
>> Choosing the appropriate compression option when using an embeded initramfs
>> can result in significant size differences in the resulting data.
>>
>> This is caused by avoiding double compression of the initramfs contents.
>> For example on my tests, choosing CONFIG_INITRAMFS_COMPRESSION_NONE when
>> compressing the kernel using XZ) results in up to 500KiB differences (9MiB to
>>  8.5MiB) in the kernel size as the dictionary will not get polluted with
>> uncomprensible data and may reuse kernel data too.
>>
>> Despite embedding an uncompressed initramfs, a user may want to allow for a
>> compressed extra initramfs to be passed using the rd system, for example to
>> boot a recovery system. Commit 9ba4bcb645898d562498ea66a0df958ef0e7a68c
>> ("initramfs: read CONFIG_RD_ variables for initramfs compression") broke
>> that behavior by making the choice based on CONFIG_RD_* instead of adding
>> CONFIG_INITRAMFS_COMPRESSION_LZ4. Saddly, CONFIG_RD_* is also used to
>> choose the supported RD compression algorithms by the kernel and a user may
>> want to suppport more than one.
>>
>> This patch also reverses 3e4e0f0a8756dade3023d1f47d50fbced7749788
>> ("initramfs: remove "compression mode" choice") restoring back the
>> "compression mode" choice and includes the CONFIG_INITRAMFS_COMPRESSION_LZ4
>> option which was never added.
>>
>> As a result the following options are added or readed affecting the embedded
>> initramfs compression:
>> INITRAMFS_COMPRESSION_NONE Do no compression
>> INITRAMFS_COMPRESSION_GZIP Compress using gzip
>> INITRAMFS_COMPRESSION_BZIP2 Compress using bzip2
>> INITRAMFS_COMPRESSION_LZMA Compress using lzma
>> INITRAMFS_COMPRESSION_XZ Compress using xz
>> INITRAMFS_COMPRESSION_LZO Compress using lzo
>> INITRAMFS_COMPRESSION_LZ4 Compress using lz4
>>
>> These depend on the corresponding CONFIG_RD_* option being set (except NONE
>> which has no dependencies).
>>
>> This patch depends on the previous one (the previous version didn't) to
>> simplify the way in which the algorithm is chosen and keep backwards
>> compatibility with the behaviour introduced by commit
>>  9ba4bcb645898d562498ea66a0df958ef0e7a68c
>>
>> Signed-off-by: Francisco Blas Izquierdo Riera (klondike) <klondike@...ndike.es>
>> Cc: P J P <ppandit@...hat.com>
>> Cc: Paul Bolle <pebolle@...cali.nl>
>> Cc: Andrew Morton <akpm@...ux-foundation.org>
> 
> Running a bisection against usr/ was not particularly convincing but
> here is basically what I am observing which used to work just fine
> before as of v4.9 and since I tracked it to this particular
> commit/patch. Here is what my build system does:
> 
> - kernel is initially configured not to have an initramfs included
> - build the user space root file system
> - re-configure the kernel to have an initramfs included
> (CONFIG_INITRAMFS_SOURCE="/path/to/romfs") and set relevant
> CONFIG_INITRAMFS options, in my case, no compression
> - kernel is re-built with these options -> kernel+initramfs image is copied
> - kernel is re-built again without these options -> kernel image is copied
> 
> Now suppose you make changes to your root filesystem, like add/remove
> applications, initramfs_data.cpio is now a stale file and go through the
> same process again:
> 
> - build the kernel without an initramfs
> - user space (re)build
> - build the kernel with an initramfs
> 
> Building a kernel without an initramfs means setting this option:
> 
> CONFIG_INITRAMFS_SOURCE=""
> 
> whereas building a kernel with an initramfs means setting these options:
> 
> CONFIG_INITRAMFS_SOURCE="/home/fainelli/work/uclinux-rootfs/romfs
> /home/fainelli/work/uclinux-rootfs/misc/initramfs.dev"
> CONFIG_INITRAMFS_ROOT_UID=1000
> CONFIG_INITRAMFS_ROOT_GID=1000
> CONFIG_INITRAMFS_COMPRESSION_NONE=y
> # CONFIG_INITRAMFS_COMPRESSION_GZIP is not set
> # CONFIG_INITRAMFS_COMPRESSION_BZIP2 is not set
> # CONFIG_INITRAMFS_COMPRESSION_LZMA is not set
> # CONFIG_INITRAMFS_COMPRESSION_XZ is not set
> # CONFIG_INITRAMFS_COMPRESSION_LZO is not set
> # CONFIG_INITRAMFS_COMPRESSION_LZ4 is not set
> CONFIG_INITRAMFS_COMPRESSION=""
> 
> Commit db2aa7fd15e857891cefbada8348c8d938c7a2bc ("initramfs: allow again
> choice of the embedded initram compression algorithm") appears
> problematic because CONFIG_INITRAMFS_COMPRESSION which is used to
> determine the initramfs cpio extension/compression is a string, and due
> to how Kconfig works, it will evaluate, in order, how to assign it.
> Setting CONFIG_INITRAMFS_COMPRESSION_NONE with
> CONFIG_INITRAMFS_SOURCE="" cannot possibly work (because of the depends
> on INITRAMFS_SOURCE!="") yet we still manage to get it assigned to
> something: ".gz" because CONFIG_RD_GZIP=y is set in my kernel, even when
> there is no initramfs being built.
> 
> So we basically end-up generating two initramfs_data.cpio* files, one
> without extension, and one with .gz. This causes usr/Makefile to track
> usr/initramfs_data.cpio.gz, and not usr/initramfs_data.cpio, that is
> also problematic after 9e3596b0c6539e28546ff7c72a06576627068353
> ("kbuild: initramfs cleanup, set target from Kconfig") because we used
> to track everything in $(targets) before that.
> 
> The end result is that the kernel with an initramfs clearly does not
> contain what we expect it to, it has a stale initramfs_data.cpio file
> built into.
> 
> So these two specific commits have just rendered the whole thing
> unworkable for me, because of how string config options and how they get
> assigned they default values work.
> 
> So right now, we are completely at the mercy of whichever was the first
> CONFIG_RD_* compression option set and the whole process which I
> described where we try to build two kernels one after the other is just
> badly broken...
> 
> Happy to test any patch, otherwise I am just considering making hiding
> INITRAMFS_COMPRESSION depends on INITRAMFS_SOURCE!= to make results more
> predictable.

OK, so here is what I just tested and this puts me back in business, I
don't think it makes sense to expose CONFIG_INITRAMFS_COMPRESSION if
initramfs is purposely disabled anyway, will submit a formal fix later
tonight:

diff --git a/usr/Kconfig b/usr/Kconfig
index c0c48507e44e..ad0543e21760 100644
--- a/usr/Kconfig
+++ b/usr/Kconfig
@@ -220,6 +220,7 @@ config INITRAMFS_COMPRESSION_LZ4
 endchoice

 config INITRAMFS_COMPRESSION
+       depends on INITRAMFS_SOURCE!=""
        string
        default ""      if INITRAMFS_COMPRESSION_NONE
        default ".gz"   if INITRAMFS_COMPRESSION_GZIP


> 
> Thoughts?
> 
>> ---
>> I'm sorry for the noise. I'm resending this as a new version because
>> Thunderbird likes to word wrap things it shouldn't be word wrapping.
>>
>> diff --git a/usr/Kconfig b/usr/Kconfig
>> index bf8e8f1..6278f13 100644
>> --- a/usr/Kconfig
>> +++ b/usr/Kconfig
>> @@ -99,8 +99,125 @@ config RD_LZ4
>>  	  Support loading of a LZ4 encoded initial ramdisk or cpio buffer
>>  	  If unsure, say N.
>>  
>> +choice
>> +	prompt "Built-in initramfs compression mode"
>> +	depends on INITRAMFS_SOURCE!=""
>> +	optional
>> +	help
>> +	  This option allows you to decide by which algorithm the builtin
>> +	  initramfs will be compressed.  Several compression algorithms are
>> +	  available, which differ in efficiency, compression and
>> +	  decompression speed.  Compression speed is only relevant
>> +	  when building a kernel.  Decompression speed is relevant at
>> +	  each boot. Also the memory usage during decompression may become
>> +	  relevant on memory constrained systems. This is usually based on the
>> +	  dictionary size of the algorithm with algorithms like XZ and LZMA
>> +	  featuring large dictionary sizes.
>> +
>> +	  High compression options are mostly useful for users who are
>> +	  low on RAM, since it reduces the memory consumption during
>> +	  boot.
>> +
>> +	  Keep in mind that your build system needs to provide the appropriate
>> +	  compression tool to compress the generated initram cpio file for
>> +	  embedding.
>> +
>> +	  If in doubt, select 'None'
>> +
>> +config INITRAMFS_COMPRESSION_NONE
>> +	bool "None"
>> +	help
>> +	  Do not compress the built-in initramfs at all. This may sound wasteful
>> +	  in space, but, you should be aware that the built-in initramfs will be
>> +	  compressed at a later stage anyways along with the rest of the kernel,
>> +	  on those architectures that support this. However, not compressing the
>> +	  initramfs may lead to slightly higher memory consumption during a
>> +	  short time at boot, while both the cpio image and the unpacked
>> +	  filesystem image will be present in memory simultaneously
>> +
>> +config INITRAMFS_COMPRESSION_GZIP
>> +	bool "Gzip"
>> +	depends on RD_GZIP
>> +	help
>> +	  Use the old and well tested gzip compression algorithm. Gzip provides
>> +	  a good balance between compression ratio and decompression speed and
>> +	  has a reasonable compression speed. It is also more likely to be
>> +	  supported by your build system as the gzip tool is present by default
>> +	  on most distros.
>> +
>> +config INITRAMFS_COMPRESSION_BZIP2
>> +	bool "Bzip2"
>> +	depends on RD_BZIP2
>> +	help
>> +	  It's compression ratio and speed is intermediate. Decompression speed
>> +	  is slowest among the choices. The initramfs size is about 10% smaller
>> +	  with bzip2, in comparison to gzip. Bzip2 uses a large amount of
>> +	  memory. For modern kernels you will need at least 8MB RAM or more for
>> +	  booting.
>> +
>> +	  If you choose this, keep in mind that you need to have the bzip2 tool
>> +	  available to be able to compress the initram.
>> +
>> +config INITRAMFS_COMPRESSION_LZMA
>> +	bool "LZMA"
>> +	depends on RD_LZMA
>> +	help
>> +	  This algorithm's compression ratio is best but has a large dictionary
>> +	  size which might cause issues in memory constrained systems.
>> +	  Decompression speed is between the other choices. Compression is
>> +	  slowest. The initramfs size is about 33% smaller with LZMA in
>> +	  comparison to gzip.
>> +
>> +	  If you choose this, keep in mind that you may need to install the xz
>> +	  or lzma tools to be able to compress the initram.
>> +
>> +config INITRAMFS_COMPRESSION_XZ
>> +	bool "XZ"
>> +	depends on RD_XZ
>> +	help
>> +	  XZ uses the LZMA2 algorithm and has a large dictionary which may cause
>> +	  problems on memory constrained systems. The initramfs size is about
>> +	  30% smaller with XZ in comparison to gzip. Decompression speed is
>> +	  better than that of bzip2 but worse than gzip and LZO. Compression is
>> +	  slow.
>> +
>> +	  If you choose this, keep in mind that you may need to install the xz
>> +	  tool to be able to compress the initram.
>> +
>> +config INITRAMFS_COMPRESSION_LZO
>> +	bool "LZO"
>> +	depends on RD_LZO
>> +	help
>> +	  It's compression ratio is the second poorest amongst the choices. The
>> +	  kernel size is about 10% bigger than gzip. Despite that, it's
>> +	  decompression speed is the second fastest and it's compression speed
>> +	  is quite fast too.
>> +
>> +	  If you choose this, keep in mind that you may need to install the lzop
>> +	  tool to be able to compress the initram.
>> +
>> +config INITRAMFS_COMPRESSION_LZ4
>> +	bool "LZ4"
>> +	depends on RD_LZ4
>> +	help
>> +	  It's compression ratio is the poorest amongst the choices. The kernel
>> +	  size is about 15% bigger than gzip; however its decompression speed
>> +	  is the fastest.
>> +
>> +	  If you choose this, keep in mind that most distros don't provide lz4
>> +	  by default which could cause a build failure.
>> +
>> +endchoice
>> +
>>  config INITRAMFS_COMPRESSION
>>  	string
>> +	default ""      if INITRAMFS_COMPRESSION_NONE
>> +	default ".gz"   if INITRAMFS_COMPRESSION_GZIP
>> +	default ".bz2"  if INITRAMFS_COMPRESSION_BZIP2
>> +	default ".lzma" if INITRAMFS_COMPRESSION_LZMA
>> +	default ".xz"   if INITRAMFS_COMPRESSION_XZ
>> +	default ".lzo"  if INITRAMFS_COMPRESSION_LZO
>> +	default ".lz4"  if INITRAMFS_COMPRESSION_LZ4
>>  	default ".gz"   if RD_GZIP
>>  	default ".lz4"  if RD_LZ4
>>  	default ".lzo"  if RD_LZO
>>
> 

-- 
Florian

Powered by blists - more mailing lists