mbox series

[RFC,0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols

Message ID 20230812000606.72319-1-CFSworks@gmail.com
Headers show
Series mtd: ubi: Enable accessing RO filesystems in UBI vols | expand

Message

Sam Edwards Aug. 12, 2023, 12:06 a.m. UTC
Hi UBI maintainers,

My target's rootfs is a read-only filesystem stored in a static UBI volume,
mounted via a "ubiblock" device after boot. I'd also like to keep the boot
files in the same filesystem, so that it's all coupled together. To that end,
I'm working on a patchset so that U-Boot can read static UBI volumes as
read-only block devices (paralleling Linux's ubiblock mechanism).

I'm very happy with how the first 3 patches in this series turned out (so I'm
not asking about them per se, though feedback is certainly welcome). The fourth
is where I got stuck: while the code definitely works, it requires bringing DM
headers into disk/part.c, which certainly will not fly in mainline. I need to
plumb this through drivers/block/blk-uclass.c to do it "properly."

Part of the problem here is that these are *volumes,* but they are (optionally)
*named.* In U-Boot's current view of block devices, it is only a partition, and
not a volume, that may have a name. These aren't "partitions" in U-Boot's view,
since a partition is a contiguous slice of a bigger block device, while these
are (logically) separate block devices.

So, I would need to invent a new function that can look up a named (sub)volume.
I also probably need to invent new syntax, so that I'm not overloading the 0:1
syntax for partitions. I'm not especially beholden to the ':', but I do want to
use the same syntax for volume numbers and volume names, to mirror Linux's
`ubi.block=x,y` syntax.

I'm also trying to reclaim the name "ubi" to refer to a UBI volume, while
U-Boot currently thinks it should refer to the presently-mounted UBIFS. In my
current series, the meaning of "ubi" depends on whether ubifs is mounted, for
backwards-compatibility. If this isn't palpable, I could consider other options
like "ubivol"/"ubiblock"/"ubiblk"/"ubistatic"/...

So, the feedback I'm hoping for would be:
1) What is a good syntax for referring to a logical volume by name or ID?
   Keeping in mind this may affect more than just UBI, if e.g. U-Boot learns to
   peer inside LVM2s in the future.
2) What should I call the block functions for looking up a block device's
   subvolume by type+parentidx+{name,ID}?
3) Is my strategy of reclaiming "ubi" sound, or should I be conceding that to
   UBIFS and using a new type name for static UBI volumes?
4) Does my choose_blksz_for_volume() function make sense, or should I always be
   using a preferred block size (like 512) if possible?

Cheers,
Sam

Sam Edwards (4):
  mtd: ubi: register UBI attachments as DM devices
  mtd: ubi: bind block device driver for static volumes
  disk: part: fall-through if "ubi" requested but ubifs not mounted
  HACK: enable access to `ubi 0:volname` block devices

 cmd/ubi.c                    |  11 +++
 disk/part.c                  |  70 +++++++++++--
 drivers/mtd/ubi/Makefile     |   1 +
 drivers/mtd/ubi/ubi-uclass.c | 184 +++++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h       |   1 +
 include/ubi_uboot.h          |   5 +
 6 files changed, 264 insertions(+), 8 deletions(-)
 create mode 100644 drivers/mtd/ubi/ubi-uclass.c

Comments

Heiko Schocher Aug. 14, 2023, 6:45 a.m. UTC | #1
Hello Sam,

many thnkas for your patchset!

On 12.08.23 02:06, Sam Edwards wrote:
> Hi UBI maintainers,
> 
> My target's rootfs is a read-only filesystem stored in a static UBI volume,
> mounted via a "ubiblock" device after boot. I'd also like to keep the boot
> files in the same filesystem, so that it's all coupled together. To that end,
> I'm working on a patchset so that U-Boot can read static UBI volumes as
> read-only block devices (paralleling Linux's ubiblock mechanism).
> 
> I'm very happy with how the first 3 patches in this series turned out (so I'm
> not asking about them per se, though feedback is certainly welcome). The fourth
> is where I got stuck: while the code definitely works, it requires bringing DM
> headers into disk/part.c, which certainly will not fly in mainline. I need to
> plumb this through drivers/block/blk-uclass.c to do it "properly."
> 
> Part of the problem here is that these are *volumes,* but they are (optionally)
> *named.* In U-Boot's current view of block devices, it is only a partition, and
> not a volume, that may have a name. These aren't "partitions" in U-Boot's view,
> since a partition is a contiguous slice of a bigger block device, while these
> are (logically) separate block devices.
> 
> So, I would need to invent a new function that can look up a named (sub)volume.
> I also probably need to invent new syntax, so that I'm not overloading the 0:1
> syntax for partitions. I'm not especially beholden to the ':', but I do want to
> use the same syntax for volume numbers and volume names, to mirror Linux's
> `ubi.block=x,y` syntax.
> 
> I'm also trying to reclaim the name "ubi" to refer to a UBI volume, while
> U-Boot currently thinks it should refer to the presently-mounted UBIFS. In my
> current series, the meaning of "ubi" depends on whether ubifs is mounted, for
> backwards-compatibility. If this isn't palpable, I could consider other options
> like "ubivol"/"ubiblock"/"ubiblk"/"ubistatic"/...
> 
> So, the feedback I'm hoping for would be:
> 1) What is a good syntax for referring to a logical volume by name or ID?
>    Keeping in mind this may affect more than just UBI, if e.g. U-Boot learns to
>    peer inside LVM2s in the future.

Yes, we should have here some generic part...

> 2) What should I call the block functions for looking up a block device's
>    subvolume by type+parentidx+{name,ID}?
> 3) Is my strategy of reclaiming "ubi" sound, or should I be conceding that to
>    UBIFS and using a new type name for static UBI volumes?
> 4) Does my choose_blksz_for_volume() function make sense, or should I always be
>    using a preferred block size (like 512) if possible?

Your patches look good to me, and yes, we have to discuss changes in disk/part.c

I added Simon here as he has much more knowledge here, hope he can comment
this part.

bye,
Heiko
> 
> Cheers,
> Sam
> 
> Sam Edwards (4):
>   mtd: ubi: register UBI attachments as DM devices
>   mtd: ubi: bind block device driver for static volumes
>   disk: part: fall-through if "ubi" requested but ubifs not mounted
>   HACK: enable access to `ubi 0:volname` block devices
> 
>  cmd/ubi.c                    |  11 +++
>  disk/part.c                  |  70 +++++++++++--
>  drivers/mtd/ubi/Makefile     |   1 +
>  drivers/mtd/ubi/ubi-uclass.c | 184 +++++++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h       |   1 +
>  include/ubi_uboot.h          |   5 +
>  6 files changed, 264 insertions(+), 8 deletions(-)
>  create mode 100644 drivers/mtd/ubi/ubi-uclass.c
>
Sam Edwards Sept. 7, 2023, 9:46 p.m. UTC | #2
Hi Heiko and Simon,

Thought I'd follow-up to keep this discussion going. The main thing I 
would like to decide first (as it lets me start relying on it in boot 
scripts) would be the UBI access syntax:

=> ls ubi 0:rootfs /boot
=> ls ubi 0:2 /boot

Do those look good? Should I be trying to mimic the accepted syntax of 
fs/ubifs/super.c:open_ubi()? Perhaps "ubi 0!rootfs" and/or "ubi 0_2"? 
Not using ':' leaves open the possibility for logical volumes (LVM2/UBI) 
to contain partitions - not that I expect anyone will want that. :)

Cheers,
Sam
Heiko Schocher Sept. 21, 2023, 6:44 a.m. UTC | #3
Hello Sam,

sorry for the late reply...

On 07.09.23 23:46, Sam Edwards wrote:
> Hi Heiko and Simon,
> 
> Thought I'd follow-up to keep this discussion going. The main thing I would like to decide first (as
> it lets me start relying on it in boot scripts) would be the UBI access syntax:
> 
> => ls ubi 0:rootfs /boot
> => ls ubi 0:2 /boot

Looks perfect for me.

> Do those look good? Should I be trying to mimic the accepted syntax of fs/ubifs/super.c:open_ubi()?
> Perhaps "ubi 0!rootfs" and/or "ubi 0_2"? Not using ':' leaves open the possibility for logical
> volumes (LVM2/UBI) to contain partitions - not that I expect anyone will want that. :)

Good question... You never know... so from my side, yes it would
be good to allow both options ... so "ubi 0:2" and "ubi 0_2",
something like if ":" is in use "ubi 0:2" else try "ubi 0_2"

Please rebase your patchset when 2023.10 is out, thanks!

bye,
Heiko