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

[Tobias Waldekranz <tobias@waldekranz.com>]

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?

>>
>> >> +{
>> >> +       struct udevice *root;
>> >
>> > Please don't use that name , as we use it for the DM root device. How
>> > about bm_parent? It isn't actually a tree of devices, so root doesn't
>> > sound right to me anyway.
>>
>> Alright, I'll change it.
>>
>> >> +       struct blk_desc *bd;
>> >> +       struct blkmap *bm;
>> >> +       int err;
>> >> +
>> >> +       if (devnum >= 0 && blkmap_from_devnum(devnum))
>> >> +               return -EBUSY;
>> >> +
>> >> +       root = blkmap_root();
>> >> +       if (!root)
>> >> +               return -ENODEV;
>> >> +
>> >> +       bm = calloc(1, sizeof(*bm));
>> >
>> > Can this be attached to the device as private data, so avoiding the malloc()?
>>
>> Maybe, I'm not familiar enough with the U-Boot internals.
>>
>> As it is now, all blkmaps are children of a single "blkmap_root"
>> device. I chose that approach so that devnums would be allocated from a
>> single pool.
>
> Well, driver model handles this for you (see dev_seq()). You have a
> single uclass so can attach your 'overall' blkmap data to that. Then
> each device can have its own private data attached.
>
> The only requirement is that BLK devices have a parent (so we know the
> media type). I had understood that you had one blkmap for each block
> map. If that is true, then you don't need to have a parent one as
> well. You can use the BLKMAP uclass to hold any overall data.
>
>>
>> AFAIK, that would mean having to store it in the "blkmap_blk" device,
>> but I thought that its private data was owned by the block subsystem?
>
> [..]
>
> Regards,
> Simon