[RFC PATCH] openbmc: Use JFFS2 for RW overlay filesystem

Wed Feb 17 18:09:50 AEDT 2016

Hi Milton,

Well - with prompts you gave me below the patch has been significantly
reduced and is much less twisty. It seems I sent myself off down rabbit
holes on several fronts.

On Tue, 2016-02-16 at 00:10 +0000, Milton Miller II wrote:
> Hi Andrew.
> 
> You got ahead of me and explored some issues, thanks.  I wasn't thinking
> about some of the details but I had plans for some others, and had been
> manually changing the type on my own system (causing someone to re-flash
> when it didn't boot for them.  cie la vie.
> 
> 
> On  02/15/2016 around 01:54AM in some timezone,  Andrew Jeffery wrote:
> > Subject: [RFC PATCH] openbmc: Use JFFS2 for RW overlay filesystem
> > 
> > We can enable JFFS2 support on mtd6 (the RW mtd partition) with a
> > (first-pass) kernel patch[1] applied to work-around a 'stutter' in the
> > (optimised) ARM mmiocpy() implementation.
> 
> optimised is flagged in my spell checker.

Hah - do we have a preference for US vs UK spelling? I don't mind.

> 
> > 
> > Whilst the original scripting surrounding the filesystem selection aimed
> > to be generic, creating and mounting JFFS2 has some warts that seems to
> > require special-case logic.
> 
> Or at least some versions of the tools think so :-)

Right - for some reason it was in my head that the mount.jffs2 helper
was required and overlooked the fact that busybox would mount it out-of
-the-box. Additionally I figured we'd want to access /dev/mtd6 rather
than go through the block emulation layer and I don't think busybox was
happy about that? Anyway, this all lead to dragging in mtd-utils-jffs2.
I've thrown that out and just used busybox.

> 
> > 
> > [1] Unmerged, but tested: https://lists.ozlabs.org/pipermail/openbmc/2016-February/001874.html
> > 
> > Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> > ---
> >  meta-phosphor/classes/image-overlay.bbclass        |  3 ++-
> >  meta-phosphor/classes/obmc-phosphor-image.bbclass  |  2 ++
> >  .../obmc-phosphor-image_types_uboot.bbclass        | 12 ++++++++++--
> >  .../obmc-phosphor-initfs/files/obmc-init.sh        | 22 +++++++++++++++-------
> >  .../obmc-phosphor-initfs/files/obmc-update.sh      | 13 +++++++++----
> >  5 files changed, 38 insertions(+), 14 deletions(-)
> > 
> > diff --git a/meta-phosphor/classes/image-overlay.bbclass b/meta-phosphor/classes/image-overlay.bbclass
> > index 1c6137d..5043e82 100644
> > --- a/meta-phosphor/classes/image-overlay.bbclass
> > +++ b/meta-phosphor/classes/image-overlay.bbclass
> > @@ -3,7 +3,8 @@ INITRD_IMAGE ?= "core-image-minimal-initramfs"
> >  INITRD ?= "${DEPLOY_DIR_IMAGE}/${INITRD_IMAGE}-${MACHINE}.cpio.${INITRD_CTYPE}${uboot}"
> >  
> >  IMAGE_BASETYPE ?= "squashfs-xz"
> > -OVERLAY_BASETYPE ?= "ext4"
> > +OVERLAY_BASETYPE ?= "jffs2"
> > +OVERLAY_BASETYPE_OPTS ?= "--pagesize=4096 --little-endian --squash"
> >  
> 
> Lots of options on that mkfs.   Not sure what the --squash flag would be for.

Hah, yeah that was some over-enthusiasm with options. Would've helped
if I actually followed through on reading what the option was for.
Instead, I've defined OVERLAY_BASETYPE_OPTS with the ext4 options and
commented it out, as it's not used unless OVERLAY_BASETYPE is not jffs2
(see below).

> 
> >  IMAGE_TYPES_${PN} += "${IMAGE_BASETYPE}"
> >  
> diff --git a/meta-phosphor/classes/obmc-phosphor-image.bbclass b/meta-phosphor/classes/obmc-phosphor-image.bbclass
> index 9a13f7f..8afa1ea 100644
> --- a/meta-phosphor/classes/obmc-phosphor-image.bbclass
> +++ b/meta-phosphor/classes/obmc-phosphor-image.bbclass
> @@ -38,4 +38,6 @@ IMAGE_INSTALL += " \
>         packagegroup-obmc-phosphor-apps-extras \
>         i2c-tools \
>         screen \
> +       mtd-utils-jffs2 \
> +       mtd-utils-misc \
>         "
> 
> You say below that there is no fsck.jffs2, so why are you adding 
> mtd-utils instead of using the busybox built-in files?  Also, the 
> u-boot utilities seem to be ok to compile with out adding these to 
> the package list.

I've dropped them, as above.

> 
> > diff --git a/meta-phosphor/classes/obmc-phosphor-image_types_uboot.bbclass b/meta-phosphor/classes/obmc-phosphor-
> > image_types_uboot.bbclass
> > index c390c36..dc69dc1 100644
> > --- a/meta-phosphor/classes/obmc-phosphor-image_types_uboot.bbclass
> > +++ b/meta-phosphor/classes/obmc-phosphor-image_types_uboot.bbclass
> > @@ -51,8 +51,16 @@ do_generate_flash() {
> >         fi
> >  
> >         oe_mkimage  "${initrd}" "${INITRD_CTYPE}" || bbfatal "oe_mkimage initrd"
> > -       dd if=/dev/zero of=${ddir}/${rwfs} bs=1k count=${RWFS_SIZE}
> > -       mkfs.${OVERLAY_BASETYPE} -b 4096 -F -O^huge_file ${ddir}/${rwfs} || bbfatal "mkfs rwfs"
> > +
> > +       if [ x"${OVERLAY_BASETYPE}" = xjffs2 ]; then
> > +              mkfs.${OVERLAY_BASETYPE} ${OVERLAY_BASETYPE_OPTS} \
> > +                     --pad=$(expr ${RWFS_SIZE} \* 1024) \
> > +                     -o ${ddir}/${rwfs} || bbfatal "mkfs rwfs"
> > +       else
> > +              dd if=/dev/zero of=${ddir}/${rwfs} bs=1k count=${RWFS_SIZE}
> > +              mkfs.${OVERLAY_BASETYPE} ${OVERLAY_BASETYPE_OPTS} \
> > +                     ${ddir}/${rwfs} || bbfatal "mkfs rwfs"
> > +       fi
> > 
> 
> That seems like a lot of logic including special caseing the type but then using the variable, 
> and some kind of --pad option.

--pad was needed to set the output to a specific size when using mtd
-utils mkfs.jffs2.

>    However, it was pointed out to me while I originally used 
> the -j option on the busybox flash_eraseall  command, which writes 1985 to each erase block, 
> the Facebook and other code just did erase_all and mount.  Investigation shows jffs2 seems 
> to be very happy with a blank NOR flash; I suspect the markers may be more for NAND and/or 
> RAM.
> 
> Also while there were some ext4 specific options not in variables they should be moved there
> before switching the type so it will be easier to go back (if, for instance, someone wants 
> to use managed nand eg emmc or usb for their read-write storage).

Yeah, done.

> 
> So, my suggestion here for empty mkfs.jffs2:
> 
> dd (size options) | tr '\x00' '\xff'

I've now unconditionally done this, and then special-cased the call to
mkfs.${OVERLAY_BASETYPE}. The only issue was tr doesn't seem to parse
'\xNN'? I've used octal instead (tr '\000' '\377').

> 
> 
> > diff --git a/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-init.sh b/meta-phosphor/common
> > /recipes-phosphor/obmc-phosphor-initfs/files/obmc-init.sh
> > index 6750de3..226b199 100644
> > --- a/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-init.sh
> > +++ b/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-init.sh
> > @@ -65,18 +65,26 @@ then
> >  > > 	> > cp /run/mtd:u-boot-env /run/fw_env
> >  fi
> >  
> > +rofst=squashfs
> >  rofs=$(findmtd rofs)
> > -rwfs=$(findmtd rwfs)
> > -
> > +roopts=ro
> >  rodev=/dev/mtdblock${rofs#mtd}
> > -rwdev=/dev/mtdblock${rwfs#mtd}
> >  
> > -rofst=squashfs
> > -rwfst=ext4
> > -roopts=ro
> > +rwfst=jffs2
> > +rwfs=$(findmtd rwfs)
> >  rwopts=rw
> > +rwdev=${rwfs}
> > +
> 
> You seem to have reordered the lines for a different grouping. 
>  That's probably ok but please do it in a seperate patch either
> before or after changing the file system type.

I re-ordered it so that the rwdev munging made (slightly) more sense.
I've now dropped all of this munging and re-arranging as we can
continue to use /dev/mtdblockX as you pointed out.

> 
> > +# mount.jffs2 requires the device node basename, not the absolute device node
> > +# path. But still support alternative filesystems through /dev/mtdblockX.
> > +if [ x"$rwfst" != xjffs2 ]; then
> > +> > 	> > rwdev=/dev/mtdblock${rwdev#mtd}
> > +fi
> 
> The kernel mount system call is very happy to accept the /dev/mtdblockX and 
> I have been doing all my testing by just changing rwfst.  If the mount.jffs2 
> that you added from mtdutils does not accept this then not only is it a waste 
> of space but actively harmeful.  
> 
> Please remove that broken code from the build and try without.  Busybox mount
> seems to be fine when the fstype is listed.
> 
> Alternatively the kernel accepts mtd: where name is the mtd name.  While 
> that is useful I have not checked if squashfs is happy with that, and it makes 
> it harder to switch to other fs types (eg loop mount).

I've dropped the change and kept rwdev as /dev/mtdblockX

> 
>  
> >  init=/sbin/init
> > +
> > +# There is no fsck for jffs2 - For rwfst=jffs2 just produces a warning in the
> > +# output about being unable to find the binary and the boot continues.
> 
> That is true, or we could create a link to /bin/true.  The comment it not 
> given to the screen where the user sees the message. 
> > > 
 Mounting read-write $rwdev filesystem failed.  Please fix and run
> > -> > 	> > mount $rwdev $rwdir -t $rwfs -o $rwopts
> > +> > 	> > mount $rwdev $rwdir -t $rwfst -o $rwopts
> >  to to continue, or do change nothing to run from RAM for this boot.
> >  HERE
> 
> Good catch, I must not have tried to cut and paste that.
> It should arguably be a seperate patch.

Done

> 
> 
> > diff --git a/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-update.sh b/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-update.sh
> > index face06d..2bba4b0 100755
> > --- a/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-update.sh
> > +++ b/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/obmc-update.sh
> > @@ -35,13 +35,18 @@ findmtd() {
> 
> [ Same comments about variable order changes and updates in update script ]
> 

Yeah, these rearrangements are no-longer part of the patch.

Cheers,

Andrew

> 
> But I also had plans to have the concept of "old" and "new" rwfst , where update
> would check for the fs mounting as the old type before but be mounted with the
> new type after the update.
> 
> Its a bit tricky because jffs2 will try to mount anything but complain about erase 
> marks on every block that it can not parse.  ext4 relies on magic numbers and so 
> should always be tried before jffs2.
> 
> Also I was thinking about taking advantage of flash_eraseall being an alias for 
> mkfs.jffs2 and doing the wipe in more cases, maybe by an alternate trigger file.  I
> don't want to do it for a u-boot-env update but probably should when we do 
> image-rofs.
> 
> 
> I have a patch series to split the whitelist into pieces that affects mostly 
> update (and a rename in init).  I didn't post it because I wanted a chance to 
> review it and had yet to investigate making moving the files in the build 
> system.  I will probably send that tomorrow.  Actually the merge looks easy its 
> all contained to the save loop block; most of the work is in the recipe.
> 
> 
> 
> Thanks,
> milton
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160217/42ddf829/attachment.sig>