Re: [PATCH/RFC 0/3] UBI: unify mouting rootfs based on cmdline parameter

[Daniel Golle <daniel@makrotopia.org>]

Hi Boris,

On Sun, Aug 28, 2016 at 05:00:54PM +0200, Boris Brezillon wrote:
> On Sun, 28 Aug 2016 16:40:14 +0200
> Daniel Golle <daniel@makrotopia.org> wrote:
> 
> > On Sun, Aug 28, 2016 at 11:25:54AM -0300, Ezequiel Garcia wrote:
> > > On 28 August 2016 at 11:20, Boris Brezillon
> > > <boris.brezillon@free-electrons.com> wrote:  
> > > > On Sun, 28 Aug 2016 11:12:50 -0300
> > > > Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> > > >  
> > > >> Daniel,
> > > >>
> > > >> Let's try to tackle this from a different angle.
> > > >>
> > > >> On 27 August 2016 at 16:43, Daniel Golle <daniel@makrotopia.org> wrote:  
> > > >> > Hi!
> > > >> >
> > > >> > In an attempts to fix the flaws of the current set of UBI-related
> > > >> > patches we are carrying in OpenWrt, I re-wrote the way mounting the
> > > >> > rootfs from UBI in OpenWrt/LEDE works. The main requirement I face
> > > >> > which cannot be easily addressed using other means which are already
> > > >> > available in the kernel is the fact that UBIFS and squashfs-on-UBI
> > > >> > require different parameters to be set on the cmdline, e.g.
> > > >> > for UBIFS: ubi.mtd=ubi root=ubi0:rootfs rootfstype=ubifs
> > > >> > for squashfs: ubi.mtd=ubi ubiblock=0,1 root=/dev/ubiblock0_1 rootfstype=squashfs
> > > >> >  
> > > >>
> > > >> Can you help me understand the problem you are solving here?
> > > >>
> > > >> So you currently need to do:
> > > >>
> > > >> * for UBIFS: ubi.mtd=ubi root=ubi0:rootfs rootfstype=ubifs
> > > >> * for squashfs: ubi.mtd=ubi ubi.block=0,1 root=/dev/ubiblock0_1
> > > >> rootfstype=squashfs
> > > >>
> > > >> [..]  
> > > >> >
> > > >> > With those changes, a single set of cmdline parameters is
> > > >> > sufficient to mount either UBIFS or any other block filesystem
> > > >> > by creating a ubiblock device:
> > > >> > ubi.mtd=ubi root=ubi0:rootfs rootfstype=ubifs,squashfs
> > > >> >  
> > > >>
> > > >> And you would like to do:
> > > >>
> > > >> * for UBIFS: ubi.mtd=ubi root=ubi0:rootfs rootfstype=ubifs
> > > >> * for squashfs: ubi.mtd=ubi root=/dev/ubiblock0_1 rootfstype=squashfs  
> > > >
> > > > I think Daniel wants something like:
> > > >
> > > > ubi.mtd=1 root=ubi0:rootfs
> > > >
> > > > or
> > > >
> > > > ubi.mtd=1 root=/dev/ubiblock0_1
> > > >
> > > > to work for both the UBIFS and squashfs cases.
> > > >  
> > > 
> > > Right. In which case, I was going to propose the same thing
> > > you just did! It would be simple, and uninvasive to introduce
> > > something like the parameter you suggested
> > > "ubi.block=attach_all_ro_vols".  
> > 
> > That in addition with the patch referred to in an earlier mail
> > http://code.bulix.org/fkxrgt-105392
> > would indeed solve the problem, with the limitation that one needs to
> > use the {ubi_num}_{vol_id} syntax instead of being able to refer to
> > the volume name.
> 
> Let me clarify my previous answer: I'm still strongly opposed to this
> approach (including my less invasive patch). I was just trying to show
> that your implementation was invasive and could be made less invasive,
> but it remains a hack, which, unless proven otherwise, is not needed.

I agree and merely wanted to discuss the vague direction rather than
details of the implementation. Using your patch, if it was merged,
would indeed at least provide a partial alternative.

Anyway, What more of a proof for the need of such a hack would be needed?
Facts:
[1] the bootloader should not need to be aware of the filesystem-type
    used for Linux' rootfs
[2] the device-tree explicitely prohibits information on fstypes
[3] disitributions and users use different filesystem types on top of
    UBI inside the same volume on the very same board
[4] two entirely different sets of parameters are needed to mount UBIFS
    or e.g. SQUASHFS (or any other read-only block filesystem)
[5] in initramfs should be used for complex boot setups

>From fact [3] and [4] you can imply that either the cmdline needs to be
adapted based on the filesystem type [6] or a new probing mechanism
needs to be introduced, either in the kernel [7] or by using an
initramfs [8].
[1] and [2] contradict with [6].
[8] seems the way to go if you consider [5] to be true in that case
(ie. "this is a complex setup"), which is what this discussion boils
down to imho.


> 
> > Currently, we do use 'dynamic' (ie. read-write) volumes even for
> > squashfs, as otherwise boot takes much longer as the CRC for the
> > whole volumes needs to be calculated. Having *any* 'static' volumes
> > also breaks some older versions of U-Boot already supporting UBI.
> > I'd rather say "attach_all_non_ubifs_vols" and probe the filesystem
> > type, though that's also not very clean.
> > Also, one might not want to attach *all* volumes, ie. for a ubootenv
> > volume, there should not be a ubiblock.
> > Re-using the volume-name parser from UBIFS to also create the ubiblock
> > device needed to mount the rootfs seemed to be the most transparent
> > approach to me.
> 
> So each time you're asking more. AFAIK, JFFS2 was only supporting
> the /dev/mtdblockX format, without any way to use MTD names. But now you
> want an improved version that is able to use the UBI format, which is
> not at all suitable for block-device probing.

I've been asking for that ("move the volume name parser from UBIFS to
UBI for it to be reused elsewhere") from the beginning of this debate.
To get to your remark: Please look at the actual patches I submitted
for discussion at the beginning of this thread.
For MTD, refering to partitions by numbers is fine, because those
numbers are static and merely a result of the partitioning scheme given
in the device-tree or cmdline mtdparts.
For UBI, volumes are dynamic and users my delete and create volumes
during the life-time of a device as they like. Thus, it's not asking
for more, it's just asking for having the same degree of reliability
and transparency as when using other storage subsystems.

> 
> To be very clear, on my side this is a NACK.

So, for the record, I conclude that those patches will remain
OpenWrt/LEDE specific and you and others on linux-mtd are opposed to
the general *approach* (and not just the actual implementation).

> 
> Regards,
> 
> Boris
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/