Patchwork [2/6] fs/ext2: add ability to build ext3/4 too

login
register
mail settings
Submitter Yann E. MORIN
Date March 7, 2013, 10:04 p.m.
Message ID <b55a35d2b1dc1f5bb2598cc8bb29f787407250bc.1362693453.git.yann.morin.1998@free.fr>
Download mbox | patch
Permalink /patch/225979/
State Changes Requested
Headers show

Comments

Yann E. MORIN - March 7, 2013, 10:04 p.m.
Use the host-e2fsprogs to tune2fs the generated rootfs.ext2 image,
and upgrade it to either one of ext2, ext3 or ext4.

Since calling tune2fs may require running e2fsck (tune2fs will warn
to do so when certain FS options are changed), we systematically call
e2fsck. This makes the code path simpler, and as a side-effect checks
that genext2fs did not generate garbage.

In turn, e2fsck will unconditionally add a UUID to the filesystem,
which is bad for reproducibility, so we call tune2fs again to remove
the UUID. This does not require checking the filesystem.

To ensure compatibility of Buildroot's .config, leave ext2 as the
default. Boards' .config can override this at will.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Peter Korsgaard <jacmet@uclibc.org>

---
Changes v1->v2: (all after comments by Arnout)
  - menuconfig prompt rework (& Peter)
  - cleanup ext2.mk
  - use environment to pass ext generation/variant
  - add rational for 1081 additional blocks
  - explain the reason we run e2fsck
---
 fs/ext2/Config.in                |   36 ++++++++++++++++----
 fs/ext2/ext2.mk                  |    7 +++-
 fs/ext2/genext2fs.sh             |   70 +++++++++++++++++++++++++++++++++++++-
 package/e2fsprogs/Config.in.host |    2 +-
 4 files changed, 105 insertions(+), 10 deletions(-)
Thomas Petazzoni - March 10, 2013, 1:52 p.m.
Dear Yann E. MORIN,

On Thu,  7 Mar 2013 23:04:39 +0100, Yann E. MORIN wrote:

>  ROOTFS_EXT2_DEPENDENCIES = host-genext2fs
> +ifeq ($(BR2_PACKAGE_HOST_E2FSPROGS),y)
> +ROOTFS_EXT2_DEPENDENCIES = host-e2fsprogs
> +endif

I believe the ifeq () here is useless: you are anyway selecting
BR2_PACKAGE_HOST_E2FSPROGS from BR2_TARGET_ROOTFS_EXT2.

Initially, I was confused by this test: it would mean that you could
potentially work without host-e2fsprogs, which isn't the case.

> +EXT2_ENV = GEN=$(BR2_TARGET_ROOTFS_EXT2_GEN)
>  
>  define ROOTFS_EXT2_CMD
> -	PATH=$(TARGET_PATH) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@
> +	PATH=$(TARGET_PATH) $(EXT2_ENV) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@
>  endef

Why PATH=$(TARGET_PATH) ? I know it was like this, but HOST_PATH would
be more appropriate. That said, it's completely silly to have both
TARGET_PATH and HOST_PATH, since they are essentially the same thing.

Also, why do you pass GEN= in the environment? Other options are passed
through normal command line options, so it's strange to move away from
this idea just for GEN=<foo>, no?

Thomas
Thomas Petazzoni - March 10, 2013, 1:58 p.m.
Dear Yann E. MORIN,

On Thu,  7 Mar 2013 23:04:39 +0100, Yann E. MORIN wrote:

> +    # e2fsck will force a UUID, which we do not want
> +    tune2fs -U clear "${IMG}"

I think you already explained that you're removing the UUID to make the
filesystem creation reproducible (i.e avoid having random bits in the
filesystem image). Even though I'm not it's entirely feasible (some
binaries inside the filesystem may contain build dates and things like
that), I think it's an interesting goal. So my only suggestion here
would be to extend a bit your comment, to explain *why* you do not want
this UUID.

Maybe we could set the volume name to 'buildroot' ? I.e:

	tune2fs -L buildroot "${IMG}"

Best regards,

Thomas
Yann E. MORIN - March 11, 2013, 12:20 a.m.
Thomas, All,

On Sunday 10 March 2013 Thomas Petazzoni wrote:
> On Thu,  7 Mar 2013 23:04:39 +0100, Yann E. MORIN wrote:
> 
> >  ROOTFS_EXT2_DEPENDENCIES = host-genext2fs
> > +ifeq ($(BR2_PACKAGE_HOST_E2FSPROGS),y)
> > +ROOTFS_EXT2_DEPENDENCIES = host-e2fsprogs
> > +endif
> 
> I believe the ifeq () here is useless: you are anyway selecting
> BR2_PACKAGE_HOST_E2FSPROGS from BR2_TARGET_ROOTFS_EXT2.

Oh, right, we just have to have an unconditional dependency.

> Initially, I was confused by this test: it would mean that you could
> potentially work without host-e2fsprogs, which isn't the case.

That was just a remnant from when e2fsprogs was a dependency only
for ext3 and ext4. Now, we need it for ext2, too.

Nice catch! :-)

> > +EXT2_ENV = GEN=$(BR2_TARGET_ROOTFS_EXT2_GEN)
> >  
> >  define ROOTFS_EXT2_CMD
> > -	PATH=$(TARGET_PATH) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@
> > +	PATH=$(TARGET_PATH) $(EXT2_ENV) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@
> >  endef
> 
> Why PATH=$(TARGET_PATH) ? I know it was like this, but HOST_PATH would
> be more appropriate. That said, it's completely silly to have both
> TARGET_PATH and HOST_PATH, since they are essentially the same thing.

Well, ask Peter! ;-)
    http://git.buildroot.org/buildroot/commit/fs/ext2/ext2.mk?id=fb951b9

> Also, why do you pass GEN= in the environment? Other options are passed
> through normal command line options, so it's strange to move away from
> this idea just for GEN=<foo>, no?

That's what I initially did, but Arnout suggested to pass it in the env
instead:
    http://lists.busybox.net/pipermail/buildroot/2013-February/067488.html
    http://lists.busybox.net/pipermail/buildroot/2013-February/067552.html

The problem I see with passing it as an option is that it is not a valid
option for genext2fs, and it means we have to reconstruct the genext2fs
options in the script, which is a bit ugly, as you can see in my previous
submission.

But I do not have a strong opinion either...

Regards,
Yann E. MORIN.
Yann E. MORIN - March 11, 2013, 12:26 a.m.
Thomas, All,

On Sunday 10 March 2013 Thomas Petazzoni wrote:
> On Thu,  7 Mar 2013 23:04:39 +0100, Yann E. MORIN wrote:
> 
> > +    # e2fsck will force a UUID, which we do not want
> > +    tune2fs -U clear "${IMG}"
> 
> I think you already explained that you're removing the UUID to make the
> filesystem creation reproducible (i.e avoid having random bits in the
> filesystem image). Even though I'm not it's entirely feasible (some
> binaries inside the filesystem may contain build dates and things like
> that), I think it's an interesting goal. So my only suggestion here
> would be to extend a bit your comment, to explain *why* you do not want
> this UUID.

Yes, will do.

> Maybe we could set the volume name to 'buildroot' ? I.e:
> 
> 	tune2fs -L buildroot "${IMG}"

Good idea, too.

Thank you!

Regards,
Yann E. MORIN.
Thomas Petazzoni - March 11, 2013, 9:21 p.m.
Dear Yann E. MORIN,

On Mon, 11 Mar 2013 01:20:28 +0100, Yann E. MORIN wrote:

> > > +EXT2_ENV = GEN=$(BR2_TARGET_ROOTFS_EXT2_GEN)
> > >  
> > >  define ROOTFS_EXT2_CMD
> > > -	PATH=$(TARGET_PATH) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@
> > > +	PATH=$(TARGET_PATH) $(EXT2_ENV) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@
> > >  endef
> > 
> > Why PATH=$(TARGET_PATH) ? I know it was like this, but HOST_PATH would
> > be more appropriate. That said, it's completely silly to have both
> > TARGET_PATH and HOST_PATH, since they are essentially the same thing.
> 
> Well, ask Peter! ;-)
>     http://git.buildroot.org/buildroot/commit/fs/ext2/ext2.mk?id=fb951b9

Huhu. We should probably have one single <something>_PATH variable,
doesn't make much sense to have HOST_PATH and TARGET_PATH. But fair
enough, that's another topic not related at all with the present patch
set.

> > Also, why do you pass GEN= in the environment? Other options are passed
> > through normal command line options, so it's strange to move away from
> > this idea just for GEN=<foo>, no?
> 
> That's what I initially did, but Arnout suggested to pass it in the env
> instead:
>     http://lists.busybox.net/pipermail/buildroot/2013-February/067488.html
>     http://lists.busybox.net/pipermail/buildroot/2013-February/067552.html
> 
> The problem I see with passing it as an option is that it is not a valid
> option for genext2fs, and it means we have to reconstruct the genext2fs
> options in the script, which is a bit ugly, as you can see in my previous
> submission.

Ok, fair enough, I'm fine with the environment variable. Not entirely
pretty, but the other option isn't either. So just let's go with the
existing code.

Thanks!

Thomas
Arnout Vandecappelle - March 12, 2013, 5:40 p.m.
On 03/10/13 14:58, Thomas Petazzoni wrote:
> Maybe we could set the volume name to 'buildroot' ? I.e:
>
> 	tune2fs -L buildroot "${IMG}"

  I don't particularly like that. If you're going to add volume names, it 
should be configurable.

  Regards,
  Arnout
Yann E. MORIN - March 12, 2013, 10:56 p.m.
Arnout, Thomas, All,

On Tuesday 12 March 2013 Arnout Vandecappelle wrote:
> On 03/10/13 14:58, Thomas Petazzoni wrote:
> > Maybe we could set the volume name to 'buildroot' ? I.e:
> >
> > 	tune2fs -L buildroot "${IMG}"
> 
>   I don't particularly like that. If you're going to add volume names, it 
> should be configurable.

It can even be a generic FS option, that can be used by all FS that can
set a label.

Material for a separate series, though; will do later.

Regards,
Yann E. MORIN.

Patch

diff --git a/fs/ext2/Config.in b/fs/ext2/Config.in
index cb4beed..c8320e2 100644
--- a/fs/ext2/Config.in
+++ b/fs/ext2/Config.in
@@ -1,10 +1,32 @@ 
 config BR2_TARGET_ROOTFS_EXT2
-	bool "ext2 root filesystem"
+	bool "ext2/3/4 root filesystem"
+	select BR2_PACKAGE_HOST_E2FSPROGS
 	help
-	  Build an ext2 root filesystem
+	  Build an ext2/3/4 root filesystem
 
 if BR2_TARGET_ROOTFS_EXT2
 
+choice
+	bool "ext2/3/4 variant"
+	default BR2_TARGET_ROOTFS_EXT2_2
+
+config BR2_TARGET_ROOTFS_EXT2_2
+	bool "ext2"
+
+config BR2_TARGET_ROOTFS_EXT2_3
+	bool "ext3"
+
+config BR2_TARGET_ROOTFS_EXT2_4
+	bool "ext4"
+
+endchoice
+
+config BR2_TARGET_ROOTFS_EXT2_GEN
+	int
+	default 2 if BR2_TARGET_ROOTFS_EXT2_2
+	default 3 if BR2_TARGET_ROOTFS_EXT2_3
+	default 4 if BR2_TARGET_ROOTFS_EXT2_4
+
 config BR2_TARGET_ROOTFS_EXT2_BLOCKS
 	int "size in blocks (leave at 0 for auto calculation)"
 	default 0
@@ -21,27 +43,27 @@  choice
 	prompt "Compression method"
 	default BR2_TARGET_ROOTFS_EXT2_NONE
 	help
-	  Select compressor for ext2 filesystem of the root filesystem
+	  Select compressor for ext2/3/4 filesystem of the root filesystem
 
 config BR2_TARGET_ROOTFS_EXT2_NONE
 	bool "no compression"
 	help
-	  Do not compress the ext2 filesystem.
+	  Do not compress the ext2/3/4 filesystem.
 
 config BR2_TARGET_ROOTFS_EXT2_GZIP
 	bool "gzip"
 	help
-	  Do compress the ext2 filesystem with gzip.
+	  Do compress the ext2/3/4 filesystem with gzip.
 
 config BR2_TARGET_ROOTFS_EXT2_BZIP2
 	bool "bzip2"
 	help
-	  Do compress the ext2 filesystem with bzip2.
+	  Do compress the ext2/3/4 filesystem with bzip2.
 
 config BR2_TARGET_ROOTFS_EXT2_LZMA
 	bool "lzma"
 	help
-	  Do compress the ext2 filesystem with lzma.
+	  Do compress the ext2/3/4 filesystem with lzma.
 
 endchoice
 
diff --git a/fs/ext2/ext2.mk b/fs/ext2/ext2.mk
index 7b71592..1660d9c 100644
--- a/fs/ext2/ext2.mk
+++ b/fs/ext2/ext2.mk
@@ -19,9 +19,14 @@  EXT2_OPTS += -m $(BR2_TARGET_ROOTFS_EXT2_RESBLKS)
 endif
 
 ROOTFS_EXT2_DEPENDENCIES = host-genext2fs
+ifeq ($(BR2_PACKAGE_HOST_E2FSPROGS),y)
+ROOTFS_EXT2_DEPENDENCIES = host-e2fsprogs
+endif
+
+EXT2_ENV = GEN=$(BR2_TARGET_ROOTFS_EXT2_GEN)
 
 define ROOTFS_EXT2_CMD
-	PATH=$(TARGET_PATH) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@
+	PATH=$(TARGET_PATH) $(EXT2_ENV) fs/ext2/genext2fs.sh -d $(TARGET_DIR) $(EXT2_OPTS) $@
 endef
 
 $(eval $(call ROOTFS_TARGET,ext2))
diff --git a/fs/ext2/genext2fs.sh b/fs/ext2/genext2fs.sh
index 7a518ae..0790729 100755
--- a/fs/ext2/genext2fs.sh
+++ b/fs/ext2/genext2fs.sh
@@ -1,10 +1,13 @@ 
 #!/bin/sh
 # genext2fs wrapper calculating needed blocks/inodes values if not specified
+set -e
 
 export LC_ALL=C
 
 CALC_BLOCKS=1
 CALC_INODES=1
+EXT_OPTS=
+EXT_OPTS_O=
 
 while getopts x:d:D:b:i:N:m:g:e:zfqUPhVv f
 do
@@ -14,6 +17,7 @@  do
 	d) TARGET_DIR=$OPTARG ;;
     esac
 done
+eval IMG="\"\${${OPTIND}}\""
 
 # calculate needed inodes
 if [ $CALC_INODES -eq 1 ];
@@ -30,7 +34,71 @@  then
     # we scale inodes / blocks with 10% to compensate for bitmaps size + slack
     BLOCKS=$(du -s -c -k $TARGET_DIR | grep total | sed -e "s/total//")
     BLOCKS=$(expr 500 + \( $BLOCKS + $INODES / 8 \) \* 11 / 10)
+    # we add 1081 blocks (a bit more than 1 MiB, assuming 1KiB blocks) for
+    # the journal if ext3/4
+    # Note: I came to 1081 blocks after trial-and-error checks. With 1080 or
+    # less additional blocks, and tune2fs would refuse to add the journal;
+    # with 1081 additional blocks or above, tune2fs wil happily add a journal.
+    # YMMV.
+    if [ ${GEN} -ge 3 ]; then
+        BLOCKS=$(expr 1081 + $BLOCKS )
+    fi
     set -- $@ -b $BLOCKS
 fi
 
-exec genext2fs $@
+e2tunefsck() {
+    # Upgrade the file system
+    if [ $# -ne 0 ]; then
+        tune2fs "$@" "${IMG}"
+    fi
+
+    # After changing filesystem options, running fsck is required
+    # (see: man tune2fs). Running e2fsck in other cases will ensure
+    # coherency of the filesystem, although it is not required.
+    # 'e2fsck -pDF -C0' means:
+    #  - automatically repair
+    #  - optimise and check for duplicate entries
+    #  - force checking
+    #  - print progress on stdout
+    # Exit codes 1 & 2 are OK, it means fs errors were successfully
+    # corrected, hence our little trick with $ret.
+    ret=0
+    e2fsck -pDf -C0 "${IMG}" >/dev/null || ret=$?
+    case ${ret} in
+       0|1|2) ;;
+       *)   exit ${ret};;
+    esac
+    printf "e2fsck was successfully run on rootfs.ext${GEN}\n"
+
+    # e2fsck will force a UUID, which we do not want
+    tune2fs -U clear "${IMG}"
+}
+
+# Check we know what generation to generate
+case "${GEN}" in
+    2|3|4)
+	;;
+    *)
+	printf "%s: unknown ext generation to generate\n" "${0##*/}" >&2
+	exit 1
+	;;
+esac
+
+# Add a journal for ext3 and above
+if [ ${GEN} -ge 3 ]; then
+    EXT_OPTS="${EXT_OPTS} -j -J size=1"
+fi
+
+# Add ext4 specific features
+if [ ${GEN} -ge 4 ]; then
+    EXT_OPTS_O="${EXT_OPTS_O},extents,uninit_bg,dir_index"
+fi
+
+# Add our -O options (there will be at most one leading comma, remove it)
+if [ -n "${EXT_OPTS_O}" ]; then
+    EXT_OPTS="${EXT_OPTS} -O ${EXT_OPTS_O#,}"
+fi
+
+# Generate and upgrade the filesystem
+genext2fs "$@"
+e2tunefsck ${EXT_OPTS}
diff --git a/package/e2fsprogs/Config.in.host b/package/e2fsprogs/Config.in.host
index ea6a0bd..0c001c2 100644
--- a/package/e2fsprogs/Config.in.host
+++ b/package/e2fsprogs/Config.in.host
@@ -1,6 +1,6 @@ 
 config BR2_PACKAGE_HOST_E2FSPROGS
 	bool "host e2fsprogs"
 	help
-	  The EXT2 file system utilities.
+	  The EXT2/3/4 file system utilities.
 	  
 	  http://e2fsprogs.sf.net