[PATCH 1/5] initramfs: move unnecessary memcmp from hot path

[PATCH 1/5] initramfs: move unnecessary memcmp from hot path

[David Disseldorp]

do_header() is called for each cpio entry and first checks for "newc"
magic before parsing further. The magic check includes a special case
error message if POSIX.1 ASCII (cpio -H odc) magic is detected. This
special case POSIX.1 check needn't be done in the hot path, so move it
under the non-newc-magic error path.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 init/initramfs.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index af27abc59643..f01590cefa2d 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -256,12 +256,11 @@ static int __init do_collect(void)
 
 static int __init do_header(void)
 {
-	if (memcmp(collected, "070707", 6)==0) {
-		error("incorrect cpio method used: use -H newc option");
-		return 1;
-	}
 	if (memcmp(collected, "070701", 6)) {
-		error("no cpio magic");
+		if (memcmp(collected, "070707", 6) == 0)
+			error("incorrect cpio method used: use -H newc option");
+		else
+			error("no cpio magic");
 		return 1;
 	}
 	parse_header(collected);
-- 
2.31.1

[PATCH 2/5] initramfs: print helpful cpio error on "crc" magic

[David Disseldorp]

Contrary to the buffer-format.rst documentation, initramfs cpio
extraction does not support "crc" archives, which carry "070702"
header magic. Make it a little clearer that "newc" (magic="070701") is
the only supported cpio format, by extending the POSIX.1 ASCII
(magic="070707") specific error message to also cover "crc" magic.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 init/initramfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index f01590cefa2d..19b1c70446fc 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -257,7 +257,7 @@ static int __init do_collect(void)
 static int __init do_header(void)
 {
 	if (memcmp(collected, "070701", 6)) {
-		if (memcmp(collected, "070707", 6) == 0)
+		if (memcmp(collected, "0707", 4) == 0)
 			error("incorrect cpio method used: use -H newc option");
 		else
 			error("no cpio magic");
-- 
2.31.1

[PATCH v2 3/5] docs: remove mention of "crc" cpio format support

[David Disseldorp]

init/initramfs.c only supports extraction of cpio archives carrying the
"newc" header magic ("070701"). Remove statements indicating support for
the "crc" cpio format.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 .../early-userspace/buffer-format.rst         | 24 +++++++------------
 1 file changed, 8 insertions(+), 16 deletions(-)

Version 2:
- reword initramfs padding description, as suggested by Matthew Wilcox

diff --git a/Documentation/driver-api/early-userspace/buffer-format.rst b/Documentation/driver-api/early-userspace/buffer-format.rst
index 7f74e301fdf3..0df76bca444c 100644
--- a/Documentation/driver-api/early-userspace/buffer-format.rst
+++ b/Documentation/driver-api/early-userspace/buffer-format.rst
@@ -14,10 +14,10 @@ is different.  The initramfs buffer contains an archive which is
 expanded into a ramfs filesystem; this document details the format of
 the initramfs buffer format.
 
-The initramfs buffer format is based around the "newc" or "crc" CPIO
-formats, and can be created with the cpio(1) utility.  The cpio
-archive can be compressed using gzip(1).  One valid version of an
-initramfs buffer is thus a single .cpio.gz file.
+The initramfs buffer format is based around the "newc" CPIO format, and
+can be created with the cpio(1) utility.  The cpio archive can be
+compressed using gzip(1).  One valid version of an initramfs buffer is
+thus a single .cpio.gz file.
 
 The full format of the initramfs buffer is defined by the following
 grammar, where::
@@ -40,9 +40,8 @@ grammar, where::
 
 
 In human terms, the initramfs buffer contains a collection of
-compressed and/or uncompressed cpio archives (in the "newc" or "crc"
-formats); arbitrary amounts zero bytes (for padding) can be added
-between members.
+compressed and/or uncompressed cpio archives (in the "newc" format),
+with arbitrary amount of zero-byte padding between members.
 
 The cpio "TRAILER!!!" entry (cpio end-of-archive) is optional, but is
 not ignored; see "handling of hard links" below.
@@ -55,7 +54,7 @@ by the ASCII string "000012ac"):
 ============= ================== ==============================================
 Field name    Field size	 Meaning
 ============= ================== ==============================================
-c_magic	      6 bytes		 The string "070701" or "070702"
+c_magic	      6 bytes		 The string "070701"
 c_ino	      8 bytes		 File inode number
 c_mode	      8 bytes		 File mode and permissions
 c_uid	      8 bytes		 File uid
@@ -68,8 +67,7 @@ c_min	      8 bytes		 Minor part of file device number
 c_rmaj	      8 bytes		 Major part of device node reference
 c_rmin	      8 bytes		 Minor part of device node reference
 c_namesize    8 bytes		 Length of filename, including final \0
-c_chksum      8 bytes		 Checksum of data field if c_magic is 070702;
-				 otherwise zero
+c_chksum      8 bytes		 Ignored; reserved for unsupported "crc" format
 ============= ================== ==============================================
 
 The c_mode field matches the contents of st_mode returned by stat(2)
@@ -78,12 +76,6 @@ on Linux, and encodes the file type and file permissions.
 The c_filesize should be zero for any file which is not a regular file
 or symlink.
 
-The c_chksum field contains a simple 32-bit unsigned sum of all the
-bytes in the data field.  cpio(1) refers to this as "crc", which is
-clearly incorrect (a cyclic redundancy check is a different and
-significantly stronger integrity check), however, this is the
-algorithm used.
-
 If the filename is "TRAILER!!!" this is actually an end-of-archive
 marker; the c_filesize for an end-of-archive marker must be zero.
 
-- 
2.31.1

[PATCH 4/5] initramfs: use do_utime() wrapper consistently

[David Disseldorp]

vfs_utimes() is called via the do_utime() wrapper everywhere except in
do_copy(). Make it consistent.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 init/initramfs.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 19b1c70446fc..7f809a1c8e89 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -378,13 +378,9 @@ static int __init do_name(void)
 static int __init do_copy(void)
 {
 	if (byte_count >= body_len) {
-		struct timespec64 t[2] = { };
 		if (xwrite(wfile, victim, body_len, &wfile_pos) != body_len)
 			error("write error");
-
-		t[0].tv_sec = mtime;
-		t[1].tv_sec = mtime;
-		vfs_utimes(&wfile->f_path, t);
+		do_utime(collected, mtime);
 
 		fput(wfile);
 		eat(body_len);
-- 
2.31.1

[PATCH 5/5] initramfs: add INITRAMFS_PRESERVE_MTIME Kconfig option

[David Disseldorp]

initramfs cpio mtime preservation, as implemented via
889d51a10712b6fd6175196626de2116858394f4, uses a linked list to defer
directory mtime processing until after all other items in the cpio
archive have been processed. This is done to ensure that parent
directory mtimes aren't overwritten via subsequent child creation.

This change adds a new INITRAMFS_PRESERVE_MTIME Kconfig option, which
can be used to disable on-by-default mtime retention and in turn
speed up initramfs extraction, particularly for cpio archives with large
directory counts.

For a cpio archive with ~1M directories, rough 20-run local benchmarks
demonstrated:
				mean extraction time (s)	std dev
INITRAMFS_PRESERVE_MTIME=y		3.789035		0.005474
INITRAMFS_PRESERVE_MTIME unset		3.111508		0.004132

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 init/Kconfig           | 10 +++++++++
 init/Makefile          |  3 +++
 init/initramfs.c       | 42 ++----------------------------------
 init/initramfs_mtime.c | 49 ++++++++++++++++++++++++++++++++++++++++++
 init/initramfs_mtime.h | 11 ++++++++++
 5 files changed, 75 insertions(+), 40 deletions(-)
 create mode 100644 init/initramfs_mtime.c
 create mode 100644 init/initramfs_mtime.h

diff --git a/init/Kconfig b/init/Kconfig
index 55f9f7738ebb..a79b6ba4d76c 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1324,6 +1324,16 @@ config BOOT_CONFIG
 
 	  If unsure, say Y.
 
+config INITRAMFS_PRESERVE_MTIME
+	bool "Preserve cpio archive mtimes in initramfs"
+	default y
+	help
+	  Each entry in an initramfs cpio archive carries an mtime value. When
+	  enabled, extracted cpio items take this mtime, with directory mtime
+	  setting deferred until after creation of any child entries.
+
+	  If unsure, say Y.
+
 choice
 	prompt "Compiler optimization level"
 	default CC_OPTIMIZE_FOR_PERFORMANCE
diff --git a/init/Makefile b/init/Makefile
index 2846113677ee..d72bf80170ce 100644
--- a/init/Makefile
+++ b/init/Makefile
@@ -11,6 +11,9 @@ obj-y                          += noinitramfs.o
 else
 obj-$(CONFIG_BLK_DEV_INITRD)   += initramfs.o
 endif
+ifeq ($(CONFIG_INITRAMFS_PRESERVE_MTIME),y)
+obj-$(CONFIG_BLK_DEV_INITRD)   += initramfs_mtime.o
+endif
 obj-$(CONFIG_GENERIC_CALIBRATE_DELAY) += calibrate.o
 
 obj-y                          += init_task.o
diff --git a/init/initramfs.c b/init/initramfs.c
index 7f809a1c8e89..205fd62be616 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -16,6 +16,8 @@
 #include <linux/namei.h>
 #include <linux/init_syscalls.h>
 
+#include "initramfs_mtime.h"
+
 static ssize_t __init xwrite(struct file *file, const char *p, size_t count,
 		loff_t *pos)
 {
@@ -115,46 +117,6 @@ static void __init free_hash(void)
 	}
 }
 
-static long __init do_utime(char *filename, time64_t mtime)
-{
-	struct timespec64 t[2];
-
-	t[0].tv_sec = mtime;
-	t[0].tv_nsec = 0;
-	t[1].tv_sec = mtime;
-	t[1].tv_nsec = 0;
-	return init_utimes(filename, t);
-}
-
-static __initdata LIST_HEAD(dir_list);
-struct dir_entry {
-	struct list_head list;
-	char *name;
-	time64_t mtime;
-};
-
-static void __init dir_add(const char *name, time64_t mtime)
-{
-	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
-	if (!de)
-		panic_show_mem("can't allocate dir_entry buffer");
-	INIT_LIST_HEAD(&de->list);
-	de->name = kstrdup(name, GFP_KERNEL);
-	de->mtime = mtime;
-	list_add(&de->list, &dir_list);
-}
-
-static void __init dir_utime(void)
-{
-	struct dir_entry *de, *tmp;
-	list_for_each_entry_safe(de, tmp, &dir_list, list) {
-		list_del(&de->list);
-		do_utime(de->name, de->mtime);
-		kfree(de->name);
-		kfree(de);
-	}
-}
-
 static __initdata time64_t mtime;
 
 /* cpio header parsing */
diff --git a/init/initramfs_mtime.c b/init/initramfs_mtime.c
new file mode 100644
index 000000000000..0020deb21f76
--- /dev/null
+++ b/init/initramfs_mtime.c
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/syscalls.h>
+#include <linux/utime.h>
+#include <linux/file.h>
+#include <linux/init_syscalls.h>
+
+#include "initramfs_mtime.h"
+
+long __init do_utime(char *filename, time64_t mtime)
+{
+	struct timespec64 t[2];
+
+	t[0].tv_sec = mtime;
+	t[0].tv_nsec = 0;
+	t[1].tv_sec = mtime;
+	t[1].tv_nsec = 0;
+	return init_utimes(filename, t);
+}
+
+static __initdata LIST_HEAD(dir_list);
+struct dir_entry {
+	struct list_head list;
+	char *name;
+	time64_t mtime;
+};
+
+void __init dir_add(const char *name, time64_t mtime)
+{
+	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
+	if (!de)
+		panic("can't allocate dir_entry buffer");
+	INIT_LIST_HEAD(&de->list);
+	de->name = kstrdup(name, GFP_KERNEL);
+	de->mtime = mtime;
+	list_add(&de->list, &dir_list);
+}
+
+void __init dir_utime(void)
+{
+	struct dir_entry *de, *tmp;
+	list_for_each_entry_safe(de, tmp, &dir_list, list) {
+		list_del(&de->list);
+		do_utime(de->name, de->mtime);
+		kfree(de->name);
+		kfree(de);
+	}
+}
diff --git a/init/initramfs_mtime.h b/init/initramfs_mtime.h
new file mode 100644
index 000000000000..6d15c8b1171f
--- /dev/null
+++ b/init/initramfs_mtime.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
+long do_utime(char *filename, time64_t mtime) __init;
+void dir_add(const char *name, time64_t mtime) __init;
+void dir_utime(void) __init;
+#else
+static long __init do_utime(char *filename, time64_t mtime) { return 0; }
+static void __init dir_add(const char *name, time64_t mtime) {}
+static void __init dir_utime(void) {}
+#endif
-- 
2.31.1

Re: [PATCH 1/5] initramfs: move unnecessary memcmp from hot path

[Chaitanya Kulkarni]

On 9/22/2021 4:52 AM, David Disseldorp wrote:
> External email: Use caution opening links or attachments
> 
> 
> do_header() is called for each cpio entry and first checks for "newc"
> magic before parsing further. The magic check includes a special case
> error message if POSIX.1 ASCII (cpio -H odc) magic is detected. This
> special case POSIX.1 check needn't be done in the hot path, so move it
> under the non-newc-magic error path.
> 

A cover letter would be nice for such a series, unless I missed it.

Re: [PATCH 1/5] initramfs: move unnecessary memcmp from hot path

[Al Viro]

On Wed, Sep 22, 2021 at 01:52:18PM +0200, David Disseldorp wrote:
> do_header() is called for each cpio entry and first checks for "newc"
> magic before parsing further. The magic check includes a special case
> error message if POSIX.1 ASCII (cpio -H odc) magic is detected. This
> special case POSIX.1 check needn't be done in the hot path, so move it
> under the non-newc-magic error path.

You keep refering to hot paths; do you have any data to support that
assertion?

How much does that series buy you on average, and what kind of dispersion
do you get before and after it?

I'm not saying I hate the patches themselves, but those references in commit
messages ping my BS detectors every time I see them ;-/

Re: [PATCH 1/5] initramfs: move unnecessary memcmp from hot path

[Chaitanya Kulkarni]

On 9/22/2021 4:35 PM, Al Viro wrote:
> On Wed, Sep 22, 2021 at 01:52:18PM +0200, David Disseldorp wrote:
>> do_header() is called for each cpio entry and first checks for "newc"
>> magic before parsing further. The magic check includes a special case
>> error message if POSIX.1 ASCII (cpio -H odc) magic is detected. This
>> special case POSIX.1 check needn't be done in the hot path, so move it
>> under the non-newc-magic error path.
> You keep refering to hot paths; do you have any data to support that
> assertion?
> 
> How much does that series buy you on average, and what kind of dispersion
> do you get before and after it?
> 
> I'm not saying I hate the patches themselves, but those references in commit
> messages ping my BS detectors every time I see them ;-/

And this exactly why cover-letter with a quantitative date for such
series is important based on my experience...

Re: [PATCH 1/5] initramfs: move unnecessary memcmp from hot path

[David Disseldorp]

On Wed, 22 Sep 2021 23:35:50 +0000, Al Viro wrote:

> On Wed, Sep 22, 2021 at 01:52:18PM +0200, David Disseldorp wrote:
> > do_header() is called for each cpio entry and first checks for "newc"
> > magic before parsing further. The magic check includes a special case
> > error message if POSIX.1 ASCII (cpio -H odc) magic is detected. This
> > special case POSIX.1 check needn't be done in the hot path, so move it
> > under the non-newc-magic error path.  
> 
> You keep refering to hot paths; do you have any data to support that
> assertion?

The code-path is run for every single initramfs entry on every single
boot. Calling that "hot" is reasonable IMO.

> How much does that series buy you on average, and what kind of dispersion
> do you get before and after it?

I withdrew the perf claims for this patch after you rightly pointed out
that my previous numbers weren't statistically significant. After doing
further checks I noticed that there was more to be gained via the
INITRAMFS_PRESERVE_MTIME changes. Numbers for extraction performance
with and without mtime preservation can be found in [PATCH 5/5] of this
series.

> I'm not saying I hate the patches themselves, but those references in commit
> messages ping my BS detectors every time I see them ;-/

I'll try harder to strip anything that might be considered buzzwordy in
future, sorry.

Cheers, David

Re: [PATCH 1/5] initramfs: move unnecessary memcmp from hot path

[David Disseldorp]

On Wed, 22 Sep 2021 23:28:04 +0000, Chaitanya Kulkarni wrote:

> A cover letter would be nice for such a series, unless I missed it.

I didn't bother, as they're pretty minor changes. I'll add one next time
if another round is needed.

Cheers, David