Re: mount root hooks for non-disk-device devices

[Simon Burge <simonb%NetBSD.org@localhost>]

Thanks Taylor and Chuck for the comments.  Replying to both emails
there.

Taylor R Campbell wrote:

> > Date: Fri, 12 Aug 2022 17:31:31 +1000
> > From: Simon Burge <simonb%NetBSD.org@localhost>
> > 
> > As part of my boot-from-ZFS work, I've generalised how "non-disk"
> > disk-type storage devices interface with the mountroot mechanism and
> > also tidied up how devices can report that they can contain a virtual
> > storage device (eg, a RAID, LVM, ZFS, etc).
>
> Nice!  Can you be sure to commit separate changes?  1. D_STORAGEPOOL,
> 2. device_has_partitions, 3. mountroot rootspec hooks?

Will do.

> >  - A new bdev/cdev device flag D_STORAGEPOOL, which indicates that the
> >    device can contain other storage device.
>
> We should have these bdevsw/cdevsw::d_flag D_* flags documented
> somewhere.  Can you go into more detail on what criteria determine
> whether to use D_STORAGEPOOL or not?

This was based on the hard-coded list of devices in the RAIDframe code,
and boils down to is it a hard-disk like device and not a floppy or tape
device.

More documentation is always good yeah...  I probably won't tackle that
as part of this change. :/

Also open to a better name than STORAGEPOOL if anyone can think of one.

> >  - A new device_t flag DVF_NO_PARTITIONS, for storage devices which
> >    don't contain partititons (eg, wedges, flash).
>
> You've added a new function device_set_flag to change the flag
> dynamically.  Why dynamically?  Why not make it part of the cfattach
> declaration?

Agreed, this is a better way.  Thanks.

> For that matter: Why is it in the device_t and not the cdevsw/bdevsw?
> (I realize that moving from one to the other might require a more
> invasive change, so don't worry about that -- just wondering about the
> design.)

RAIDframe (and my ZFS code) both use deviter(9), so querying via
the device_t seemed logical.  I'll go back and look at this.

>
> +void *
> +mountroot_rootspec_hook_establish(const char *prefix,
> +    device_t (*hookfn)(const char *), void (*printfn)(void))
>
> This should return a non-void pointer, e.g. struct
> mountroot_rootspec_hook *.  It doesn't have to be exposed in a header
> file -- a forward declaration `struct mountroot_rootspec_hook;' in the
> header file is enough.
>
> (We should do the same for other things like softint_establish and
> pci_intr_establish.  No reason to abuse void * when we can have
> distinct named types.)

To catch void * misuse?  Will do that.

> +       hd = malloc(sizeof(*hd), M_DEVBUF, M_NOWAIT);
>
> Use kmem, not malloc.

I just followed existing convention in kern_hook.c.  I'll adjust the
other users (in a separate commit).  See question below about NOWAIT
that applies to other calls in kern_hook.c.

> +	LIST_INSERT_HEAD(&rootspec_hook_list, hd, hk_list);
>
> Seems to me this should check for duplicates and panic if so.

What's the reasoning behind that?  Would that check be
#ifdef DIAGNOSTIC ?

> +device_bdev_flags(device_t dv)
>
> Maybe this should just be device_is_storagepool or something?  I'm not
> sure exposing the mapping from device_t to the bdevsw flags is
> generally useful.

That's reasonable, will do that.

Chuck Silvers wrote:

> On Fri, Aug 12, 2022 at 05:31:31PM +1000, Simon Burge wrote:
> >  - A new bdev/cdev device flag D_STORAGEPOOL, which indicates that the
> >    device can contain other storage device.
>
> I see from the diff that this is replacing the list of device type names
> in raidframe's rf_find_raid_components().  but I don't think that this change
> is actually an improvement.  it moves the knowledge of what kinds of disk devices
> that raidframe wants to examine to look for its labels out of the raidframe code
> and into every other block device driver.  if you really want to get rid of
> this list of device types in the raidframe code then we should find
> a better way to express the intent.

The idea here was that RAIDframe and ZFS both was to know what disk
devices to probe during autoconfiguration.  Originally I had pretty
much the same hard coded list in the ZFS before this change.  Having
duplicated lists in consumers seemed not optimal.

Do you have any alternative suggestions for other ways to deal with
this?

> +mountroot_rootspec_hook_establish(const char *prefix,
> +    device_t (*hookfn)(const char *), void (*printfn)(void))
> +{
> +       struct rootspec_hook_desc *hd;
> +
> +       hd = malloc(sizeof(*hd), M_DEVBUF, M_NOWAIT);
> +       if (hd == NULL)
> +               return (NULL);
>
> is there any reason for this to be nowait?  all memory allocations should be
> M_WAITOK / KM_SLEEP unless there's a good reason why you have to use nowait.

As per my reply to Taylor's above, I just copied existing convention in
kern_hook.c.  I don't have an otherwise good reason to keep NOWAIT, but
can't comment on the other two uses in this file.  I assume they can all
sleep?  At least for mine, I will use KM_SLEEP and kmem_alloc().
 
Cheers,
Simon.