From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753162AbdIHEgb (ORCPT ); Fri, 8 Sep 2017 00:36:31 -0400 Received: from mail-oi0-f41.google.com ([209.85.218.41]:35051 "EHLO mail-oi0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750853AbdIHEg3 (ORCPT ); Fri, 8 Sep 2017 00:36:29 -0400 X-Google-Smtp-Source: AOwi7QDEDs1WZqZ2urjCEO3j5lRRi5u99KI2gwFNLt0mZk6cLEukrSlg3ozKSJrWsdYweMUeliuY7w== Subject: Re: [PATCH] initramfs: Fix disabling of initramfs (and its compression) To: Nicholas Piggin Cc: "Francisco Blas Izquierdo Riera (klondike)" , linux-kernel@vger.kernel.org, ppandit@redhat.com, pebolle@tiscali.nl, mmarek@suse.cz, Andrew Morton , Daniel Thompson References: <20170521033337.6197-1-f.fainelli@gmail.com> <20170522124652.1bcd7749@roar.ozlabs.ibm.com> <65695e3a-e471-5bdb-868c-63adc8b0ddf8@gmail.com> <20170829130916.27054261@roar.ozlabs.ibm.com> <20170907231908.2f9a14ef@roar.ozlabs.ibm.com> From: Florian Fainelli Message-ID: <291e6b0b-2260-e93a-bd28-053df87f3d9a@gmail.com> Date: Thu, 7 Sep 2017 21:36:25 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170907231908.2f9a14ef@roar.ozlabs.ibm.com> Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/07/2017 06:19 AM, Nicholas Piggin wrote: > On Thu, 7 Sep 2017 05:50:30 -0700 > Florian Fainelli wrote: > >> On 08/28/2017 08:09 PM, Nicholas Piggin wrote: >>> On Mon, 28 Aug 2017 13:03:31 -0700 >>> Florian Fainelli wrote: >>> >>>> On 05/21/2017 07:46 PM, Nicholas Piggin wrote: >>>>> On Sat, 20 May 2017 20:33:35 -0700 >>>>> Florian Fainelli 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 >>>>> >>>>> 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 >>>> >>>> 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! 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