Patchwork [4/4] fs/ext: add support for ext2 rev0 and rev1

login
register
mail settings
Submitter Yann E. MORIN
Date Feb. 17, 2013, 11:10 p.m.
Message ID <2f41f484f574512df6919967f641cf2152049a22.1361142401.git.yann.morin.1998@free.fr>
Download mbox | patch
Permalink /patch/221115/
State Changes Requested
Headers show

Comments

Yann E. MORIN - Feb. 17, 2013, 11:10 p.m.
Some bootloaders have a buggy ext2 support, and require ext2 rev1
instead of the traditional ext2 rev0 that genext2fs produces.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 fs/ext/Config.in   |   19 ++++++++++++-------
 fs/ext/genextfs.sh |   38 +++++++++++++++++++++++++++-----------
 2 files changed, 39 insertions(+), 18 deletions(-)
Arnout Vandecappelle - Feb. 19, 2013, 7:55 a.m.
On 18/02/13 00:10, Yann E. MORIN wrote:
> Some bootloaders have a buggy ext2 support, and require ext2 rev1
> instead of the traditional ext2 rev0 that genext2fs produces.
>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> ---
>   fs/ext/Config.in   |   19 ++++++++++++-------
>   fs/ext/genextfs.sh |   38 +++++++++++++++++++++++++++-----------
>   2 files changed, 39 insertions(+), 18 deletions(-)
>
> diff --git a/fs/ext/Config.in b/fs/ext/Config.in
> index 1a7a315..be0ed9c 100644
> --- a/fs/ext/Config.in
> +++ b/fs/ext/Config.in
> @@ -7,10 +7,14 @@ if BR2_TARGET_ROOTFS_EXT
>
>   choice
>   	bool "ext generation"
> -	default BR2_TARGET_ROOTFS_EXT_EXT2
> +	default BR2_TARGET_ROOTFS_EXT_EXT2r0
>
> -config BR2_TARGET_ROOTFS_EXT_EXT2
> -	bool "ext2"
> +config BR2_TARGET_ROOTFS_EXT_EXT2r0
> +	bool "ext2 (rev0)"
> +
> +config BR2_TARGET_ROOTFS_EXT_EXT2r1
> +	bool "ext2 (rev1)"
> +	select BR2_PACKAGE_HOST_E2FSPROGS
>
>   config BR2_TARGET_ROOTFS_EXT_EXT3
>   	bool "ext3"
> @@ -23,10 +27,11 @@ config BR2_TARGET_ROOTFS_EXT_EXT4
>   endchoice
>
>   config BR2_TARGET_ROOTFS_EXT_GEN
> -	int
> -	default 2 if BR2_TARGET_ROOTFS_EXT_EXT2
> -	default 3 if BR2_TARGET_ROOTFS_EXT_EXT3
> -	default 4 if BR2_TARGET_ROOTFS_EXT_EXT4
> +	string
> +	default 2.0 if BR2_TARGET_ROOTFS_EXT_EXT2r0
> +	default 2.1 if BR2_TARGET_ROOTFS_EXT_EXT2r1
> +	default 3   if BR2_TARGET_ROOTFS_EXT_EXT3
> +	default 4   if BR2_TARGET_ROOTFS_EXT_EXT4

  I think it makes things simpler if you keep the GEN as it is, and 
process the rev separately. Or perhaps set

config BR2_TARGET_ROOTFS_EXT_REV
	int
	default 0 if BR2_TARGET_ROOTFS_EXT_EXT2r0
	default 1

>
>   config BR2_TARGET_ROOTFS_EXT_BLOCKS
>   	int "size in blocks (leave at 0 for auto calculation)"
> diff --git a/fs/ext/genextfs.sh b/fs/ext/genextfs.sh
> index fcbd43c..ef47a25 100755
> --- a/fs/ext/genextfs.sh
> +++ b/fs/ext/genextfs.sh
> @@ -6,11 +6,13 @@ export LC_ALL=C
>
>   CALC_BLOCKS=1
>   CALC_INODES=1
> +EXT2_REV=0
>
>   while getopts x:d:D:b:i:N:m:g:e:zfqUPhVv234 f
>   do
>       case $f in
> -	2|3|4) GEN=$f ;;
> +	2.*) GEN=2; EXT2_REV=${f#*.} ;;
> +	3|4) GEN=$f ;;
>   	b) CALC_BLOCKS=0 ;;
>   	N) CALC_INODES=0; INODES=$OPTARG ;;
>   	d) TARGET_DIR=$OPTARG ;;
> @@ -56,25 +58,39 @@ for o; do
>       esac
>   done
>
> +ext_fsck() {
> +    gen="${1}"
> +    img="${2}"
> +    ret=0
> +    fsck.ext${gen} -pDf "${img}" >/dev/null || ret=$?

  Don't bother with ${gen}, just use e2fsck.

> +    # Exit codes 1 & 2 are OK, it means fs errors
> +    # were successfully corrected
> +    case ${ret} in
> +	0|1|2) ;;
> +	*)   exit 1;;
> +    esac
> +    # fsck.ext* will force a UUID, which we do not want
> +    tune2fs -U clear "${img}" >/dev/null
> +}
> +
>   # Generate the base ext2 file system
>   genext2fs "$@"
>
> +# Upgrade to ext2 rev1 if needed
> +if [ ${EXT2_REV} -ge 1 -o ${GEN} -ge 3 ]; then

  With BR2_TARGET_ROOTFS_EXT_REV this condition becomes simpler.

> +    tune2fs -O filetype "${IMG}" >/dev/null
> +    ext_fsck 2 "${IMG}"
> +fi
> +
>   # Upgrade to ext3 if needed
>   if [ ${GEN} -ge 3 ]; then
>       tune2fs -j -J size=1 "${IMG}" >/dev/null
> +    ext_fsck 3 "${IMG}"
>   fi
>
>   # Upgrade to ext4 if needed
>   if [ ${GEN} -ge 4 ]; then
>       tune2fs -O extents,uninit_bg,dir_index "${IMG}" >/dev/null
> -    ret=0
> -    fsck.ext4 -pDf "${IMG}" >/dev/null || ret=$?
> -    # Exit codes 1 & 2 are OK, it means fs errors
> -    # were successfully corrected
> -    case ${ret} in
> -	0|1|2) ;;
> -	*)   exit 1;;
> -    esac
> -    # fsck.ext4 will force a UUID, which we do not want
> -    tune2fs -U clear "${IMG}" >/dev/null
> +    NEED_FSCK=1

  I guess you originally had just a single fsck and used this variable to 
decide if it was needed. That's actually a good idea.

  Regards,
  Arnout

> +    ext_fsck 4 "${IMG}"
>   fi
>
Yann E. MORIN - Feb. 19, 2013, 6:10 p.m.
Arnout, All,

On Tuesday 19 February 2013 Arnout Vandecappelle wrote:
> On 18/02/13 00:10, Yann E. MORIN wrote:
> > Some bootloaders have a buggy ext2 support, and require ext2 rev1
> > instead of the traditional ext2 rev0 that genext2fs produces.
[--SNIP--]
> > @@ -23,10 +27,11 @@ config BR2_TARGET_ROOTFS_EXT_EXT4
> >   endchoice
> >
> >   config BR2_TARGET_ROOTFS_EXT_GEN
> > -	int
> > -	default 2 if BR2_TARGET_ROOTFS_EXT_EXT2
> > -	default 3 if BR2_TARGET_ROOTFS_EXT_EXT3
> > -	default 4 if BR2_TARGET_ROOTFS_EXT_EXT4
> > +	string
> > +	default 2.0 if BR2_TARGET_ROOTFS_EXT_EXT2r0
> > +	default 2.1 if BR2_TARGET_ROOTFS_EXT_EXT2r1
> > +	default 3   if BR2_TARGET_ROOTFS_EXT_EXT3
> > +	default 4   if BR2_TARGET_ROOTFS_EXT_EXT4
> 
>   I think it makes things simpler if you keep the GEN as it is, and 
> process the rev separately. Or perhaps set
> 
> config BR2_TARGET_ROOTFS_EXT_REV
> 	int
> 	default 0 if BR2_TARGET_ROOTFS_EXT_EXT2r0
> 	default 1

Not sure. ext2 rev1 is just that: a revision 1 ext2. ext3 or ext4 rev1
do not mean anything, AFAIK.

[--SNIP--]
> > +ext_fsck() {
> > +    gen="${1}"
> > +    img="${2}"
> > +    ret=0
> > +    fsck.ext${gen} -pDf "${img}" >/dev/null || ret=$?
> 
>   Don't bother with ${gen}, just use e2fsck.

OK, I've just checked, and e2fsck's behavior does indeed not depend
on its argv[0], so I'll use that.

[--SNIP--]
> > +# Upgrade to ext2 rev1 if needed
> > +if [ ${EXT2_REV} -ge 1 -o ${GEN} -ge 3 ]; then
> 
>   With BR2_TARGET_ROOTFS_EXT_REV this condition becomes simpler.

Yes, but as I said above, rev0/1 is only meaningfull for ext2, not ext3/4.

I'd rather keep the semantics clear. The ext filesystem can be:
  - ext2 rev0
  - ext2 rev1
  - ext3
  - ext4

And the fsck is only needed for ext2 rev1, ext3 or ext4.

But it is not stupid to always run the fsck anyway, so I'll simplify the
code by always calling e2fsck, even if it is not strictly required. Doing
so will also help catching badly generated ext2 rev0 filesystems, so it's
a net gain (and does not cost too much, anyway).

> > +    tune2fs -O filetype "${IMG}" >/dev/null
> > +    ext_fsck 2 "${IMG}"
> > +fi
> > +
> >   # Upgrade to ext3 if needed
> >   if [ ${GEN} -ge 3 ]; then
> >       tune2fs -j -J size=1 "${IMG}" >/dev/null
> > +    ext_fsck 3 "${IMG}"
> >   fi
> >
> >   # Upgrade to ext4 if needed
> >   if [ ${GEN} -ge 4 ]; then
> >       tune2fs -O extents,uninit_bg,dir_index "${IMG}" >/dev/null
> > -    ret=0
> > -    fsck.ext4 -pDf "${IMG}" >/dev/null || ret=$?
> > -    # Exit codes 1 & 2 are OK, it means fs errors
> > -    # were successfully corrected
> > -    case ${ret} in
> > -	0|1|2) ;;
> > -	*)   exit 1;;
> > -    esac
> > -    # fsck.ext4 will force a UUID, which we do not want
> > -    tune2fs -U clear "${IMG}" >/dev/null
> > +    NEED_FSCK=1
> 
>   I guess you originally had just a single fsck and used this variable to 
> decide if it was needed. That's actually a good idea.

Oh, I forgot to remove that variable. We can't run fsck once at the end.
It has to be called after each tune2fs call.

Also, I'll rework the code as thus:

    ext_opts=""
    ext_opts_O=""
    if ! ext2rev0:
        ext_opts_O+="filetype"
    if ext3:
        ext_opts+="-j -J size=1"
    if ext4:
        ext_opts_O+="extents,uninit_bg,dir_index"
    if ext_opts_O != "":
        ext_opts+="-O ext_opts_O"
    tune2fs ext_opts img
    e2fsck img

That is even better, I think.

Regards,
Yann E. MORIN.
Arnout Vandecappelle - Feb. 19, 2013, 11:40 p.m.
On 19/02/13 19:10, Yann E. MORIN wrote:
> Arnout, All,
>
> On Tuesday 19 February 2013 Arnout Vandecappelle wrote:
>> On 18/02/13 00:10, Yann E. MORIN wrote:
>>> Some bootloaders have a buggy ext2 support, and require ext2 rev1
>>> instead of the traditional ext2 rev0 that genext2fs produces.
> [--SNIP--]
>>> @@ -23,10 +27,11 @@ config BR2_TARGET_ROOTFS_EXT_EXT4
>>>    endchoice
>>>
>>>    config BR2_TARGET_ROOTFS_EXT_GEN
>>> -	int
>>> -	default 2 if BR2_TARGET_ROOTFS_EXT_EXT2
>>> -	default 3 if BR2_TARGET_ROOTFS_EXT_EXT3
>>> -	default 4 if BR2_TARGET_ROOTFS_EXT_EXT4
>>> +	string
>>> +	default 2.0 if BR2_TARGET_ROOTFS_EXT_EXT2r0
>>> +	default 2.1 if BR2_TARGET_ROOTFS_EXT_EXT2r1
>>> +	default 3   if BR2_TARGET_ROOTFS_EXT_EXT3
>>> +	default 4   if BR2_TARGET_ROOTFS_EXT_EXT4
>>
>>    I think it makes things simpler if you keep the GEN as it is, and
>> process the rev separately. Or perhaps set
>>
>> config BR2_TARGET_ROOTFS_EXT_REV
>> 	int
>> 	default 0 if BR2_TARGET_ROOTFS_EXT_EXT2r0
>> 	default 1
>
> Not sure. ext2 rev1 is just that: a revision 1 ext2. ext3 or ext4 rev1
> do not mean anything, AFAIK.

  They are partly independent - although ext4 is not possible with 
rev0... And in practice you want rev1 for ext3 filesystems as well. 
Actually, you always want rev1, except when you have a stupid bootloader 
that doesn't support it.

  So what I meant is: set revision to 1, except for ext2r0.

>
> [--SNIP--]
>>> +ext_fsck() {
>>> +    gen="${1}"
>>> +    img="${2}"
>>> +    ret=0
>>> +    fsck.ext${gen} -pDf "${img}" >/dev/null || ret=$?
>>
>>    Don't bother with ${gen}, just use e2fsck.
>
> OK, I've just checked, and e2fsck's behavior does indeed not depend
> on its argv[0], so I'll use that.
>
> [--SNIP--]
>>> +# Upgrade to ext2 rev1 if needed
>>> +if [ ${EXT2_REV} -ge 1 -o ${GEN} -ge 3 ]; then
>>
>>    With BR2_TARGET_ROOTFS_EXT_REV this condition becomes simpler.
>
> Yes, but as I said above, rev0/1 is only meaningfull for ext2, not ext3/4.
>
> I'd rather keep the semantics clear. The ext filesystem can be:
>    - ext2 rev0
>    - ext2 rev1
>    - ext3
>    - ext4
>
> And the fsck is only needed for ext2 rev1, ext3 or ext4.
>
> But it is not stupid to always run the fsck anyway, so I'll simplify the
> code by always calling e2fsck, even if it is not strictly required. Doing
> so will also help catching badly generated ext2 rev0 filesystems, so it's
> a net gain (and does not cost too much, anyway).
>
>>> +    tune2fs -O filetype "${IMG}" >/dev/null
>>> +    ext_fsck 2 "${IMG}"
>>> +fi
>>> +
>>>    # Upgrade to ext3 if needed
>>>    if [ ${GEN} -ge 3 ]; then
>>>        tune2fs -j -J size=1 "${IMG}" >/dev/null
>>> +    ext_fsck 3 "${IMG}"
>>>    fi
>>>
>>>    # Upgrade to ext4 if needed
>>>    if [ ${GEN} -ge 4 ]; then
>>>        tune2fs -O extents,uninit_bg,dir_index "${IMG}" >/dev/null
>>> -    ret=0
>>> -    fsck.ext4 -pDf "${IMG}" >/dev/null || ret=$?
>>> -    # Exit codes 1 & 2 are OK, it means fs errors
>>> -    # were successfully corrected
>>> -    case ${ret} in
>>> -	0|1|2) ;;
>>> -	*)   exit 1;;
>>> -    esac
>>> -    # fsck.ext4 will force a UUID, which we do not want
>>> -    tune2fs -U clear "${IMG}" >/dev/null
>>> +    NEED_FSCK=1
>>
>>    I guess you originally had just a single fsck and used this variable to
>> decide if it was needed. That's actually a good idea.
>
> Oh, I forgot to remove that variable. We can't run fsck once at the end.
> It has to be called after each tune2fs call.
>
> Also, I'll rework the code as thus:
>
>      ext_opts=""
>      ext_opts_O=""
>      if ! ext2rev0:
>          ext_opts_O+="filetype"
>      if ext3:
>          ext_opts+="-j -J size=1"
>      if ext4:
>          ext_opts_O+="extents,uninit_bg,dir_index"
>      if ext_opts_O != "":
>          ext_opts+="-O ext_opts_O"
>      tune2fs ext_opts img
>      e2fsck img
>
> That is even better, I think.

  Agreed! Moving to Python is certainly better! :-)

  BTW doesn't tune2fs support -O x -O y options?

  Regards,
  Arnout
Yann E. MORIN - Feb. 19, 2013, 11:47 p.m.
Arnout, All,

On Wednesday 20 February 2013 Arnout Vandecappelle wrote:
> On 19/02/13 19:10, Yann E. MORIN wrote:
[--SNIP--]
> > Not sure. ext2 rev1 is just that: a revision 1 ext2. ext3 or ext4 rev1
> > do not mean anything, AFAIK.
>   They are partly independent - although ext4 is not possible with 
> rev0... And in practice you want rev1 for ext3 filesystems as well. 
> Actually, you always want rev1, except when you have a stupid bootloader 
> that doesn't support it.
> 
>   So what I meant is: set revision to 1, except for ext2r0.

Even ext3 is always rev1, anyway.

Anyway, I've reworked the code, and am doing some tests. On track for
being pushed by the end of the week...

[--SNIP--]
> > Also, I'll rework the code as thus:
> >
> >      ext_opts=""
> >      ext_opts_O=""
> >      if ! ext2rev0:
> >          ext_opts_O+="filetype"
> >      if ext3:
> >          ext_opts+="-j -J size=1"
> >      if ext4:
> >          ext_opts_O+="extents,uninit_bg,dir_index"
> >      if ext_opts_O != "":
> >          ext_opts+="-O ext_opts_O"
> >      tune2fs ext_opts img
> >      e2fsck img
> >
> > That is even better, I think.
> 
>   Agreed! Moving to Python is certainly better! :-)
> 
>   BTW doesn't tune2fs support -O x -O y options?

Yes, probably, that was just a quick-and-dirty pseudo code for the sake
of illustration. ;-)

Regards,
Yann E. MORIN, going to sleep in a short while...

Patch

diff --git a/fs/ext/Config.in b/fs/ext/Config.in
index 1a7a315..be0ed9c 100644
--- a/fs/ext/Config.in
+++ b/fs/ext/Config.in
@@ -7,10 +7,14 @@  if BR2_TARGET_ROOTFS_EXT
 
 choice
 	bool "ext generation"
-	default BR2_TARGET_ROOTFS_EXT_EXT2
+	default BR2_TARGET_ROOTFS_EXT_EXT2r0
 
-config BR2_TARGET_ROOTFS_EXT_EXT2
-	bool "ext2"
+config BR2_TARGET_ROOTFS_EXT_EXT2r0
+	bool "ext2 (rev0)"
+
+config BR2_TARGET_ROOTFS_EXT_EXT2r1
+	bool "ext2 (rev1)"
+	select BR2_PACKAGE_HOST_E2FSPROGS
 
 config BR2_TARGET_ROOTFS_EXT_EXT3
 	bool "ext3"
@@ -23,10 +27,11 @@  config BR2_TARGET_ROOTFS_EXT_EXT4
 endchoice
 
 config BR2_TARGET_ROOTFS_EXT_GEN
-	int
-	default 2 if BR2_TARGET_ROOTFS_EXT_EXT2
-	default 3 if BR2_TARGET_ROOTFS_EXT_EXT3
-	default 4 if BR2_TARGET_ROOTFS_EXT_EXT4
+	string
+	default 2.0 if BR2_TARGET_ROOTFS_EXT_EXT2r0
+	default 2.1 if BR2_TARGET_ROOTFS_EXT_EXT2r1
+	default 3   if BR2_TARGET_ROOTFS_EXT_EXT3
+	default 4   if BR2_TARGET_ROOTFS_EXT_EXT4
 
 config BR2_TARGET_ROOTFS_EXT_BLOCKS
 	int "size in blocks (leave at 0 for auto calculation)"
diff --git a/fs/ext/genextfs.sh b/fs/ext/genextfs.sh
index fcbd43c..ef47a25 100755
--- a/fs/ext/genextfs.sh
+++ b/fs/ext/genextfs.sh
@@ -6,11 +6,13 @@  export LC_ALL=C
 
 CALC_BLOCKS=1
 CALC_INODES=1
+EXT2_REV=0
 
 while getopts x:d:D:b:i:N:m:g:e:zfqUPhVv234 f
 do
     case $f in
-	2|3|4) GEN=$f ;;
+	2.*) GEN=2; EXT2_REV=${f#*.} ;;
+	3|4) GEN=$f ;;
 	b) CALC_BLOCKS=0 ;;
 	N) CALC_INODES=0; INODES=$OPTARG ;;
 	d) TARGET_DIR=$OPTARG ;;
@@ -56,25 +58,39 @@  for o; do
     esac
 done
 
+ext_fsck() {
+    gen="${1}"
+    img="${2}"
+    ret=0
+    fsck.ext${gen} -pDf "${img}" >/dev/null || ret=$?
+    # Exit codes 1 & 2 are OK, it means fs errors
+    # were successfully corrected
+    case ${ret} in
+	0|1|2) ;;
+	*)   exit 1;;
+    esac
+    # fsck.ext* will force a UUID, which we do not want
+    tune2fs -U clear "${img}" >/dev/null
+}
+
 # Generate the base ext2 file system
 genext2fs "$@"
 
+# Upgrade to ext2 rev1 if needed
+if [ ${EXT2_REV} -ge 1 -o ${GEN} -ge 3 ]; then
+    tune2fs -O filetype "${IMG}" >/dev/null
+    ext_fsck 2 "${IMG}"
+fi
+
 # Upgrade to ext3 if needed
 if [ ${GEN} -ge 3 ]; then
     tune2fs -j -J size=1 "${IMG}" >/dev/null
+    ext_fsck 3 "${IMG}"
 fi
 
 # Upgrade to ext4 if needed
 if [ ${GEN} -ge 4 ]; then
     tune2fs -O extents,uninit_bg,dir_index "${IMG}" >/dev/null
-    ret=0
-    fsck.ext4 -pDf "${IMG}" >/dev/null || ret=$?
-    # Exit codes 1 & 2 are OK, it means fs errors
-    # were successfully corrected
-    case ${ret} in
-	0|1|2) ;;
-	*)   exit 1;;
-    esac
-    # fsck.ext4 will force a UUID, which we do not want
-    tune2fs -U clear "${IMG}" >/dev/null
+    NEED_FSCK=1
+    ext_fsck 4 "${IMG}"
 fi