[v3,0/1] mke2fs: the -d option can now handle tarball input

Hi Andreas,

thanks a lot for your second review!! :)

> The big question is whether Ted will accept it.

My hope rests in the fact that Ted did not (yet) absolutely refuse to add such
a feature in their comments to the github pull request [1]. XD

[1] https://github.com/tytso/e2fsprogs/pull/118

> These parts of the change is specific to your system and should probably
> be removed from the patch.

Ah thank you for that clarification. I was a bit puzzled that you have a
generated file tracked by git and was unsure what the policy is for updating
it. This should be fixed now.

> Rather than having an inline #ifdef here, this could be structured like
> the following in create_file_libarchive.c:

I now see that you already tried to tell me how you'd like to see this in an
earlier mail but I didn't understand what you wanted to tell me. Thank you for
spelling it out for me. I hope my changes now look as you expected. I agree
that it looks much better now.

> If we expect larger files, having a 1MB or 16MB buffer is not outrageous
> these days, and would probably improve the performance noticeably.

In my own use-case I want to convert tarballs containing chroot
environments into an ext4 image. I'm on a fairly limited system (ARM
Cortex A53 @ 1.5 GHz and 4 GB RAM) and loading up a YouTube video in
Firefox takes about half a minute until it starts playing. So I'd expect
that if buffer size has any meaningful impact it would show up as a
significant difference on my system with its limited resources. So I
converted a chroot tarball of 429 MB to an ext4 image 10 times for each
buffer size and averaged the runtime as well as recorded the fastest of
the 10 runs. These are the results:

COPY_FILE_BUFLEN  time in s  min in s
65536             145.576    14.240
1048576           144.866    14.098
16777216          147.142    14.317

What else can I test? It seems that a 1 MB buffer is slightly fastest.
But I still didn't finish that part of the patch because I'm not sure
whether my benchmark should be improved or whether it looks different on
other platforms.

Thanks!

cheers, josch



Johannes Schauer Marin Rodrigues (1):
  mke2fs: the -d option can now handle tarball input

 MCONFIG.in                     |   1 +
 configure                      |  52 +++
 configure.ac                   |   9 +
 debugfs/Makefile.in            |  25 +-
 lib/config.h.in                |   3 +
 lib/ext2fs/Makefile.in         |  25 +-
 misc/Makefile.in               |  17 +-
 misc/create_inode.c            |  45 ++-
 misc/create_inode.h            |  10 +
 misc/create_inode_libarchive.c | 699 +++++++++++++++++++++++++++++++++
 misc/create_inode_libarchive.h |  10 +
 misc/mke2fs.8.in               |  10 +-
 misc/mke2fs.c                  |  12 +-
 tests/m_rootgnutar/expect      | 141 +++++++
 tests/m_rootgnutar/output.sed  |   5 +
 tests/m_rootgnutar/script      | 125 ++++++
 tests/m_rootpaxtar/expect      |  87 ++++
 tests/m_rootpaxtar/mkpaxtar.pl |  69 ++++
 tests/m_rootpaxtar/output.sed  |   5 +
 tests/m_rootpaxtar/script      |  44 +++
 tests/m_roottar/expect         | 208 ++++++++++
 tests/m_roottar/mktar.pl       |  62 +++
 tests/m_roottar/output.sed     |   5 +
 tests/m_roottar/script         |  57 +++
 24 files changed, 1691 insertions(+), 35 deletions(-)
 create mode 100644 misc/create_inode_libarchive.c
 create mode 100644 misc/create_inode_libarchive.h
 create mode 100644 tests/m_rootgnutar/expect
 create mode 100644 tests/m_rootgnutar/output.sed
 create mode 100644 tests/m_rootgnutar/script
 create mode 100644 tests/m_rootpaxtar/expect
 create mode 100644 tests/m_rootpaxtar/mkpaxtar.pl
 create mode 100644 tests/m_rootpaxtar/output.sed
 create mode 100644 tests/m_rootpaxtar/script
 create mode 100644 tests/m_roottar/expect
 create mode 100644 tests/m_roottar/mktar.pl
 create mode 100644 tests/m_roottar/output.sed
 create mode 100644 tests/m_roottar/script

Quoting Johannes Schauer Marin Rodrigues (2023-08-15 19:57:16)
> > Rather than having an inline #ifdef here, this could be structured like the
> > following in create_file_libarchive.c:
> 
> I now see that you already tried to tell me how you'd like to see this in an
> earlier mail but I didn't understand what you wanted to tell me. Thank you for
> spelling it out for me. I hope my changes now look as you expected. I agree
> that it looks much better now.

whoops, my refactoring accidentally killed the ability to build without
archive.h. Please imagine my latest patch with the following on top. The github
pull request contains the fixed version and the github actions succeed now.
Sorry for the noise!

diff --git a/misc/create_inode_libarchive.c b/misc/create_inode_libarchive.c
index c147828f..deed65e8 100644
--- a/misc/create_inode_libarchive.c
+++ b/misc/create_inode_libarchive.c
@@ -20,6 +20,8 @@
 #include "create_inode.h"
 #include "support/nls-enable.h"
 
+#ifdef HAVE_ARCHIVE_H
+
 /* 64KiB is the minimum blksize to best minimize system call overhead. */
 //#define COPY_FILE_BUFLEN 65536
 //#define COPY_FILE_BUFLEN 1048576
@@ -536,6 +538,7 @@ static errcode_t handle_entry(ext2_filsys fs, ext2_ino_t root_ino,
        }
        return 0;
 }
+#endif
 
 errcode_t __populate_fs_from_tar(ext2_filsys fs, ext2_ino_t root_ino,
                                 const char *source_tar, ext2_ino_t root,