On Tue, Oct 24, 2023 at 01:44:08AM +0000, Klemens Nanni wrote:
> Rereading the code, I now question why it checks the 'a' label type at all.
>
> Taking your sd0d example through devboot():
>
> |#ifdef SOFTRAID
> | /*
> | * Determine the partition type for the 'a' partition of the
> | * boot device.
> | */
> | TAILQ_FOREACH(dip, &disklist, list)
> | if (dip->bios_info.bios_number == bootdev &&
> | (dip->bios_info.flags & BDI_BADLABEL) == 0)
> | part_type = dip->disklabel.d_partitions[0].p_fstype;
>
> Whatever sd0a's type is...
>
> |
> | /*
> | * See if we booted from a disk that is a member of a bootable
> | * softraid volume.
> | */
> | SLIST_FOREACH(bv, &sr_volumes, sbv_link) {
> | if (bv->sbv_flags & BIOC_SCBOOTABLE)
> | SLIST_FOREACH(bc, &bv->sbv_chunks, sbc_link)
> | if (bc->sbc_disk == bootdev)
>
> ... the boot loader sees we booted from a bootable softraid chunk,
> matching disk sd0 by number and not partition.
>
> | sr_boot_vol = bv->sbv_unit;
> | if (sr_boot_vol != -1)
> | break;
> | }
> |#endif
> |
> | if (sr_boot_vol != -1 && part_type != FS_BSDFFS) {
>
> With softraid chunk on sd0d, sd0a happens to be not FFS (likely unused),
> but that's still enough for the boot loader to stick to softraid on sd0!
>
> | *p++ = 's';
> | *p++ = 'r';
> | *p++ = '0' + sr_boot_vol;
> | } else if (bootdev & 0x100) {
>
> So why check the 'a' label type if that's not relevant?
The only reason I can see for that check is to better support the unusual
configuration of booting from a disk which happens to be part of a bootable
softraid volume, but which also has a regular FFS partition outside of the
RAID, and for whatever reason the user wants to automatically boot from that
instead.
Maybe that was useful before support for booting from softraid crypto volumes
was introduced, (when it was raid-1 only). It doesn't seem very useful now.
> It is confusing and
> Doubling down on the assumption that bootable chunks are always on 'a'
> like my diff did shows that's a mistake and dropping the check actually
> makes more sense to me now.
I agree it is confusing.
Effectively the code is there to treat part_type == FS_BSDFFS an 'edge case'
as described above.
> This boots with root on softraid on sd0a and sd0d.
>
> Thoughts?
> Am I missing something?
It works OK here, so I'm fine with removing this.
