Message-ID: <65695e3a-e471-5bdb-868c-63adc8b0ddf8@gmail.com>
Date:   Mon, 28 Aug 2017 13:03:31 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Nicholas Piggin <npiggin@...il.com>,
        "Francisco Blas Izquierdo Riera (klondike)" <klondike@...ndike.es>
Cc:     linux-kernel@...r.kernel.org, ppandit@...hat.com,
        pebolle@...cali.nl, mmarek@...e.cz,
        Andrew Morton <akpm@...ux-foundation.org>,
        Daniel Thompson <daniel.thompson@...aro.org>
Subject: Re: [PATCH] initramfs: Fix disabling of initramfs (and its
 compression)

On 05/21/2017 07:46 PM, Nicholas Piggin wrote:
> On Sat, 20 May 2017 20:33:35 -0700
> Florian Fainelli <f.fainelli@...il.com> wrote:
> 
>> Commit db2aa7fd15e8 ("initramfs: allow again choice of the embedded
>> initram compression algorithm") introduced the possibility to select the
>> initramfs compression algorithm from Kconfig and while this is a nice
>> feature it broke the use case described below.
>>
>> 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 option
>> (CONFIG_INITRAMFS_COMPRESSION_NONE)
>> - kernel is re-built with these options -> kernel+initramfs image is
>>   copied
>> - kernel is re-built again without these options -> kernel image is
>>   copied
>>
>> Building a kernel without an initramfs means setting this option:
>>
>> CONFIG_INITRAMFS_SOURCE="" (and this one only)
>>
>> 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=""
>>
>> Commit db2aa7fd15e857891cefbada8348c8d938c7a2bc ("initramfs: allow again
>> choice of the embedded initram compression algorithm") is problematic
>> because CONFIG_INITRAMFS_COMPRESSION which is used to determine the
>> initramfs_data.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!="" imposed on CONFIG_INITRAMFS_COMPRESSION ) yet we
>> still get CONFIG_INITRAMFS_COMPRESSION assigned to ".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 anymore,
>> that is also largely problematic after
>> 9e3596b0c6539e28546ff7c72a06576627068353 ("kbuild: initramfs cleanup,
>> set target from Kconfig") because we used to track all possible
>> initramfs_data files in the $(targets) variable before that commit.
>>
>> 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 it, and we keep re-generating an initramfs_data.cpio.gz file
>> which is not the one that we want to include in the kernel image proper.
>>
>> The fix consists in hiding CONFIG_INITRAMFS_COMPRESSION when
>> CONFIG_INITRAMFS_SOURCE="". This puts us back in a state to the pre-4.10
>> behavior where we can properly disable and re-enable initramfs within
>> the same kernel .config file, and be in control of what
>> CONFIG_INITRAMFS_COMPRESSION is set to.
>>
>> Fixes: db2aa7fd15e8 ("initramfs: allow again choice of the embedded initram compression algorithm")
>> Fixes: 9e3596b0c653 ("kbuild: initramfs cleanup, set target from Kconfig")
>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
> 
> This is very thorough, thank you for tracking it down and fixing it.
> 
> I can't say I've worked through the problem in the code, but your
> changelog and the proposed fix seem reasonable to me. So for what
> it's worth:
> 
> Acked-by: Nicholas Piggin <npiggin@...il.com>

Well, I am looking at this again, and it's still broken, the same test
case is involved, except this time, I am switching beween no-initramfs
and initramfs with gzip compression (the key thing is using a
compression of some sort). The end result is the following:

- change stuff in the rootfs
- build the kernel with initramfs, CONFIG_INITRAMFS_COMPRESSION_GZIP=y,
usr/initramfs_data.cpio.gz gets generated correctly the first time
- build the kernel without initramfs,
CONFIG_INITRAMFS_COMPRESSION_NONE=y, usr/initramfs_data.cpio gets generated

Now back to step 1 add some files, and we can see that
usr/initramfs_data.cpio.gz is now stale from before...

So while my earlier fix switched the initramfs w/o compression to no
initramfs rebuild, now this does not work because we still have two
files left to be tracked:

usr/initramfs_data.cpio (no compression, or when initramfs is disabled)
and usr/initramfs_data.cpio.$(suffix_y)

How would you go about solving this?



> 
>> ---
>>  usr/Kconfig | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> 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
> 


-- 
Florian

Powered by blists - more mailing lists