Re: [PATCH] initramfs: Fix disabling of initramfs (and its compression)

[Florian Fainelli <f.fainelli@gmail.com>]

On 09/07/2017 09:36 PM, Florian Fainelli wrote:
> 
> 
> On 09/07/2017 06:19 AM, Nicholas Piggin wrote:
>> On Thu, 7 Sep 2017 05:50:30 -0700
>> Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>> On 08/28/2017 08:09 PM, Nicholas Piggin wrote:
>>>> On Mon, 28 Aug 2017 13:03:31 -0700
>>>> Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>   
>>>>> On 05/21/2017 07:46 PM, Nicholas Piggin wrote:  
>>>>>> On Sat, 20 May 2017 20:33:35 -0700
>>>>>> Florian Fainelli <f.fainelli@gmail.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@gmail.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@gmail.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?  
>>>>
>>>> I don't see the problem. When I change back to .gz, modify the source
>>>> directory, and rebuild, it rebuilds a new initramfs_data.cpio.gz here.  
>>>
>>> This is really puzzling me, so here is what is going on my side:
>>>
>>> First time build w/ initramfs enabled:
>>>
>>> grep INITRAMFS .config
>>> 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 is not set
>>> CONFIG_INITRAMFS_COMPRESSION_GZIP=y
>>> # 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=".gz"
>>>
>>> fainelli@fainelli-laptop:[~/../linux]$ ls -la usr
>>> total 6.8M
>>> drwxrwxr-x  2 fainelli fainelli 4.0K Sep  7 05:37 ./
>>> drwxrwxr-x 27 fainelli fainelli  16K Sep  7 05:38 ../
>>> -rw-rw-r--  1 fainelli fainelli  146 Sep  7 05:37 built-in.o
>>> -rw-rw-r--  1 fainelli fainelli  112 Sep  7 05:37 .built-in.o.cmd
>>> -rwxrwxr-x  1 fainelli fainelli  23K Sep  7 05:36 gen_init_cpio*
>>> -rw-rw-r--  1 fainelli fainelli  13K Oct 13  2016 gen_init_cpio.c
>>> -rw-rw-r--  1 fainelli fainelli 3.3K Sep  7 05:36 .gen_init_cpio.cmd
>>> -rw-rw-r--  1 fainelli fainelli  151 Aug 28  2016 .gitignore
>>> -rw-rw-r--  1 fainelli fainelli 9.6K Sep  7 05:36 .initramfs_data.cpio.d
>>> -rw-rw-r--  1 fainelli fainelli 3.3M Sep  7 05:37 initramfs_data.cpio.gz
>>> -rw-rw-r--  1 fainelli fainelli  220 Sep  7 05:37
>>> .initramfs_data.cpio.gz.cmd
>>> -rw-rw-r--  1 fainelli fainelli 3.3M Sep  7 05:37 initramfs_data.o
>>> -rw-rw-r--  1 fainelli fainelli 2.7K Sep  7 05:37 .initramfs_data.o.cmd
>>> -rw-rw-r--  1 fainelli fainelli 1.3K Aug 28  2016 initramfs_data.S
>>> -rw-rw-r--  1 fainelli fainelli 7.8K Aug 18 19:25 Kconfig
>>> -rw-rw-r--  1 fainelli fainelli 2.0K Aug 31 20:46 Makefile
>>> -rw-rw-r--  1 fainelli fainelli    0 Sep  7 05:34 modules.builtin
>>> -rw-rw-r--  1 fainelli fainelli    0 Sep  7 05:35 modules.order
>>>
>>> Now let's reconfigure this kernel w/o initramfs and rebuild:
>>>
>>> grep INITRAMFS .config
>>> CONFIG_INITRAMFS_SOURCE=""
>>>
>>> fainelli@fainelli-laptop:[~/../linux]$ ls -la usr
>>> total 3.6M
>>> drwxrwxr-x  2 fainelli fainelli 4.0K Sep  7 05:44 ./
>>> drwxrwxr-x 27 fainelli fainelli  16K Sep  7 05:44 ../
>>> -rw-rw-r--  1 fainelli fainelli  146 Sep  7 05:44 built-in.o
>>> -rw-rw-r--  1 fainelli fainelli  112 Sep  7 05:44 .built-in.o.cmd
>>> -rwxrwxr-x  1 fainelli fainelli  23K Sep  7 05:36 gen_init_cpio*
>>> -rw-rw-r--  1 fainelli fainelli  13K Oct 13  2016 gen_init_cpio.c
>>> -rw-rw-r--  1 fainelli fainelli 3.3K Sep  7 05:36 .gen_init_cpio.cmd
>>> -rw-rw-r--  1 fainelli fainelli  151 Aug 28  2016 .gitignore
>>> -rw-rw-r--  1 fainelli fainelli  512 Sep  7 05:44 initramfs_data.cpio
>>> -rw-rw-r--  1 fainelli fainelli  105 Sep  7 05:44 .initramfs_data.cpio.cmd
>>> -rw-rw-r--  1 fainelli fainelli   52 Sep  7 05:44 .initramfs_data.cpio.d
>>> -rw-rw-r--  1 fainelli fainelli 3.3M Sep  7 05:37 initramfs_data.cpio.gz
>>> -rw-rw-r--  1 fainelli fainelli  220 Sep  7 05:37
>>> .initramfs_data.cpio.gz.cmd
>>> -rw-rw-r--  1 fainelli fainelli 1.3K Sep  7 05:44 initramfs_data.o
>>> -rw-rw-r--  1 fainelli fainelli 2.7K Sep  7 05:44 .initramfs_data.o.cmd
>>> -rw-rw-r--  1 fainelli fainelli 1.3K Aug 28  2016 initramfs_data.S
>>> -rw-rw-r--  1 fainelli fainelli 7.8K Aug 18 19:25 Kconfig
>>> -rw-rw-r--  1 fainelli fainelli 2.0K Aug 31 20:46 Makefile
>>> -rw-rw-r--  1 fainelli fainelli    0 Sep  7 05:34 modules.builtin
>>> -rw-rw-r--  1 fainelli fainelli    0 Sep  7 05:35 modules.order
>>>
>>> Now let's rebuild with a new application added to the root filesystem:
>>>
>>> grep INITRAMFS .config
>>> 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 is not set
>>> CONFIG_INITRAMFS_COMPRESSION_GZIP=y
>>> # 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=".gz"
>>>
>>> We are not fine here:
>>> scripts/kconfig/conf  --silentoldconfig Kconfig
>>> #
>>> # configuration written to .config
>>> #
>>>   CHK     include/config/kernel.release
>>>   CHK     include/generated/uapi/linux/version.h
>>>   CHK     scripts/mod/devicetable-offsets.h
>>>   CHK     include/generated/utsrelease.h
>>>   CHK     include/generated/timeconst.h
>>>   CHK     include/generated/bounds.h
>>>   CHK     include/generated/asm-offsets.h
>>>   CALL    scripts/checksyscalls.sh
>>>   CHK     include/generated/compile.h
>>>   AS      usr/initramfs_data.o
>>>
>>> ^===== we should have called gzip to re-generate the initramfs, we did not
>>>
>>>   AR      usr/built-in.o
>>>   GEN     .version
>>>   CHK     include/generated/compile.h
>>>   UPD     include/generated/compile.h
>>>   CC      init/version.o
>>>   AR      init/built-in.o
>>>   AR      built-in.o
>>>   LD      vmlinux.o
>>>   MODPOST vmlinux.o
>>>
>>> And the timestamps here confirm that:
>>>
>>> fainelli@fainelli-laptop:[~/../linux]$ ls -la usr
>>> total 6.9M
>>> drwxrwxr-x  2 fainelli fainelli 4.0K Sep  7 05:46 ./
>>> drwxrwxr-x 27 fainelli fainelli  16K Sep  7 05:46 ../
>>> -rw-rw-r--  1 fainelli fainelli  146 Sep  7 05:46 built-in.o
>>> -rw-rw-r--  1 fainelli fainelli  112 Sep  7 05:46 .built-in.o.cmd
>>> -rwxrwxr-x  1 fainelli fainelli  23K Sep  7 05:36 gen_init_cpio*
>>> -rw-rw-r--  1 fainelli fainelli  13K Oct 13  2016 gen_init_cpio.c
>>> -rw-rw-r--  1 fainelli fainelli 3.3K Sep  7 05:36 .gen_init_cpio.cmd
>>> -rw-rw-r--  1 fainelli fainelli  151 Aug 28  2016 .gitignore
>>> -rw-rw-r--  1 fainelli fainelli  512 Sep  7 05:44 initramfs_data.cpio
>>> -rw-rw-r--  1 fainelli fainelli  105 Sep  7 05:44 .initramfs_data.cpio.cmd
>>> -rw-rw-r--  1 fainelli fainelli  11K Sep  7 05:46 .initramfs_data.cpio.d
>>> -rw-rw-r--  1 fainelli fainelli 3.3M Sep  7 05:37 initramfs_data.cpio.gz
>>> -rw-rw-r--  1 fainelli fainelli  220 Sep  7 05:37
>>> .initramfs_data.cpio.gz.cmd
>>> -rw-rw-r--  1 fainelli fainelli 3.3M Sep  7 05:46 initramfs_data.o
>>> -rw-rw-r--  1 fainelli fainelli 2.7K Sep  7 05:46 .initramfs_data.o.cmd
>>> -rw-rw-r--  1 fainelli fainelli 1.3K Aug 28  2016 initramfs_data.S
>>> -rw-rw-r--  1 fainelli fainelli 7.8K Aug 18 19:25 Kconfig
>>> -rw-rw-r--  1 fainelli fainelli 2.0K Aug 31 20:46 Makefile
>>> -rw-rw-r--  1 fainelli fainelli    0 Sep  7 05:46 modules.builtin
>>> -rw-rw-r--  1 fainelli fainelli    0 Sep  7 05:46 modules.order
>>>
>>> Worse yet, usr/.initramfs_data.cpio.d does list the binary that was just
>>> added, but for some reason, this did not trigger a rebuild of
>>> initramfs_data.cpio.gz do you see any reasons why?
>>
>> This rule  
>>
>> $(obj)/$(datafile_y): $(obj)/gen_init_cpio $(deps_initramfs) klibcdirs
>>         $(Q)$(initramfs) -l $(ramfs-input) > $(obj)/.initramfs_data.cpio.d
>>         $(call if_changed,initfs)
>>
>> does not have FORCE on the end, so I guess is not picking up the dependency
>> properly, perhaps?
> 
> I tried that:
> 
> diff --git a/usr/Makefile b/usr/Makefile
> index 0b87e71c00fc..28a958bebd27 100644
> --- a/usr/Makefile
> +++ b/usr/Makefile
> @@ -51,6 +51,6 @@ $(deps_initramfs): klibcdirs
>  # 2) There are changes in which files are included (added or deleted)
>  # 3) If gen_init_cpio are newer than initramfs_data.cpio
>  # 4) arguments to gen_initramfs.sh changes
> -$(obj)/$(datafile_y): $(obj)/gen_init_cpio $(deps_initramfs) klibcdirs
> +$(obj)/$(datafile_y): $(obj)/gen_init_cpio $(deps_initramfs) klibcdirs
> FORCE
>         $(Q)$(initramfs) -l $(ramfs-input) > $(obj)/.initramfs_data.cpio.d
>         $(call if_changed,initf
> 
> but this did not change anything.
> 
> We are re-generating .initramfs_data.cpio.d and its timestamp is clearly
> updated so it's got to be cmd_initfs which does not get invoked.
> 
> .initramfs_data.cpio.gz.cmd is correct:
> 
> cmd_usr/initramfs_data.cpio.gz := /bin/bash
> ./scripts/gen_initramfs_list.sh -o usr/initramfs_data.cpio.gz  -u 1000
> -g 1000  /home/fainelli/work/uclinux-rootfs/romfs
> /home/fainelli/work/uclinux-rootfs/misc/initramfs.dev
> 
> and was generated the first time we did generate the gzip initramfs, so
> the command has not changed, nor its arguments, so we just don't call it.
> 
> I did try to use if_changed_dep, which rightfully pointed that we don't
> have a .initramfs_data.cpio.gz.d, but this gets us very close from the
> only thing that I could get working thus far. Another way to possibly
> solve it is to void generating a cmd_$($@) and use the same name
> irrespective of the compression type used, that should force a change
> everytime...
> 
> Thanks Nick!

Nick, is the patch below acceptable to you?

> 
> diff --git a/usr/Makefile b/usr/Makefile
> index 0b87e71c00fc..2d7d7f4b91f1 100644
> --- a/usr/Makefile
> +++ b/usr/Makefile
> @@ -7,6 +7,7 @@ PHONY += klibcdirs
> 
>  suffix_y = $(subst $\",,$(CONFIG_INITRAMFS_COMPRESSION))
>  datafile_y = initramfs_data.cpio$(suffix_y)
> +datafile_d_y = .$(datafile_y).d
>  AFLAGS_initramfs_data.o += -DINITRAMFS_IMAGE="usr/$(datafile_y)"
> 
> 
> @@ -29,12 +30,12 @@ ramfs-args  := \
>          $(if $(CONFIG_INITRAMFS_ROOT_UID), -u
> $(CONFIG_INITRAMFS_ROOT_UID)) \
>          $(if $(CONFIG_INITRAMFS_ROOT_GID), -g $(CONFIG_INITRAMFS_ROOT_GID))
> 
> -# .initramfs_data.cpio.d is used to identify all files included
> +# $(datafile_d_y) is used to identify all files included
>  # in initramfs and to detect if any files are added/removed.
>  # Removed files are identified by directory timestamp being updated
>  # The dependency list is generated by gen_initramfs.sh -l
> -ifneq ($(wildcard $(obj)/.initramfs_data.cpio.d),)
> -       include $(obj)/.initramfs_data.cpio.d
> +ifneq ($(wildcard $(obj)/$(datafile_d_y)),)
> +       include $(obj)/$(datafile_d_y)
>  endif
> 
>  quiet_cmd_initfs = GEN     $@
> @@ -52,5 +53,5 @@ $(deps_initramfs): klibcdirs
>  # 3) If gen_init_cpio are newer than initramfs_data.cpio
>  # 4) arguments to gen_initramfs.sh changes
>  $(obj)/$(datafile_y): $(obj)/gen_init_cpio $(deps_initramfs) klibcdirs
> -       $(Q)$(initramfs) -l $(ramfs-input) > $(obj)/.initramfs_data.cpio.d
> +       $(Q)$(initramfs) -l $(ramfs-input) > $(obj)/$(datafile_d_y)
>         $(call if_changed,initfs)
> 

-- 
Florian