Re: [PATCH 3/8] blk: blkmap: Add basic infrastructure

7 Feb 2023

      On mån, feb 06, 2023 at 21:02, Simon Glass sjg@chromium.org wrote:
...
Hi Tobias,
On Mon, 6 Feb 2023 at 01:30, Tobias Waldekranz tobias@waldekranz.com wrote:
...
On fre, feb 03, 2023 at 17:20, Simon Glass sjg@chromium.org wrote:
...
Hi Tobias,
On Fri, 3 Feb 2023 at 02:38, Tobias Waldekranz tobias@waldekranz.com wrote:
...
On ons, feb 01, 2023 at 13:20, Simon Glass sjg@chromium.org wrote:
...
Hi Tobias,
Hi Simon,
Thanks for the review!
...
On Wed, 1 Feb 2023 at 11:10, Tobias Waldekranz tobias@waldekranz.com wrote:
...
blkmaps are loosely modeled on Linux's device mapper subsystem. The
basic idea is that you can create virtual block devices whose blocks
can be backed by a plethora of sources that are user configurable.
This change just adds the basic infrastructure for creating and
removing blkmap devices. Subsequent changes will extend this to add
support for actual mappings.
Signed-off-by: Tobias Waldekranz tobias@waldekranz.com
MAINTAINERS                      |   6 +
 disk/part.c                      |   1 +
 drivers/block/Kconfig            |  18 ++
 drivers/block/Makefile           |   1 +
 drivers/block/blk-uclass.c       |   1 +
 drivers/block/blkmap.c           | 275 +++++++++++++++++++++++++++++++
 include/blkmap.h                 |  15 ++
 include/dm/uclass-id.h           |   1 +
 include/efi_loader.h             |   4 +
 lib/efi_loader/efi_device_path.c |  30 ++++
 10 files changed, 352 insertions(+)
 create mode 100644 drivers/block/blkmap.c
 create mode 100644 include/blkmap.h
[..]
...
...
This needs to be created as part of DM.  See how host_create_device()
works. It attaches something to the uclass and then creates child
devices from there. It also operations (struct host_ops) but you don't
need to do that.
Note that the host commands support either an label or a devnum, which
I think is useful, so you might copy that?
I took a look at the hostfs implementation. I agree that labels are much
nicer than bare integers. However, for block maps the idea is to fit in
to the existing filesystem infrastructure. Addressing block devices
using the "<interface> <dev>[:<part>]" pattern seems very well
established...
You can still do that, so long as the labels are "0" and "1", etc. But
it lets us move to a more flexible system in future.
...
...
...
+{

  static struct udevice *dev;

  int err;

  if (dev)

          return dev;

  err = device_bind_driver(dm_root(), "blkmap_root", "blkmap", &dev);

  if (err)

          return NULL;

  err = device_probe(dev);

  if (err) {

          device_unbind(dev);

          return NULL;

  }

Should not be needed as probing children will cause this to be probed.
So this function just becomes
uclass_first_device(UCLASS_BLKDEV, &
...

  return dev;

+}

+int blkmap_create(int devnum)
Again, please drop the use of devnum and use devices. Here you could
use a label, perhaps?
...which is why I don't think a label is going to fly here. Let's say I
create a new ramdisk with a label instead, e.g.:
blkmap create rd
blkmap map rd 0 0x100 mem ${loadaddr}

How do I know which <dev> to supply to, e.g.:
ls blkmap <dev> /boot

It seems like labels are a hostfs-specific feature, or am I missing
something?
We have the same problem with hostfs, since we have not implemented
labels in block devices. For now you must use integer labels. But we
will get there.
But there is no connection to the devnum that is allocated internally by
U-Boot. Here's an experiment I just ran:
I created two squashfs images containing a single directory:
zero.squashfs:
 i_am_zero

one.squashfs:
 i_am_one

Then I added a binding to them:
=> host bind 1 one.squashfs
=> host bind 0 zero.squashfs

When accessing them, we see that the existing filesystem utilities work
on the internally generated devnums, ignoring the labels:
=> ls host 1
            i_am_zero/

0 file(s), 1 dir(s)

=> ls host 0
            i_am_one/

0 file(s), 1 dir(s)

=>

Doesn't it therefore make more sense to stick with the established
abstraction?
It is pretty clear that this is a bit silly though :-)
As in "this specific example will never be used in practice"? Sure :)
My point was just that the approach to stick to integer labels is
brittle, since there is no connection between the label and the devnum
used by existing commands.
Here's an even simpler example that might actually trip up a user:
=> host bind 1 one.squashfs
    => ls host 1
    ** Bad device specification host 1 **
    Couldn't find partition host 1
    => ls host 0
                i_am_one/
0 file(s), 1 dir(s)
=>
...
I mean, right now, it would be easier to stick with numbered devices.
But we want to be able to support named devices (e.g. using struct
udevice->name). A good way to be forward compatible is to support a
label today.
When we do get to it, the less code that has piled up and needs
converting, the more likely it is to happen.
I completely understand, and agree with, the direction you want to take
this.
...
Sure, you have the problem as above, but mostly people are only going
to use one of them, so it doesn't matter.
We could also have a way of obtaining a number from a label, if you
want to go that far. But I suggest just telling people to use labels
like "1" and "0" which should work.
Alright, well, if that is acceptable then I'll do it that way. For my
own piece of mind, I think I'll also add some way of safely doing the
reverse mapping for any label. Does the following look ok?
blkmap get <label> dev <var>
This way you could extend it with other attributes in the future
(e.g. size).