10/24/23 14:03, Crystal Kolipe пишет:
> 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.
>
That matches sys/arch/amd64/stand/libsa/dev_i386.c r1.12 from 2012
which introduced this code; my second diff would be a revert:
If we have booted from a disk that is a member of a bootable
softraid volume, always select the srX device unless the 'a'
partition of the disk is FFS.
(Other architectures copied that, except sparc64 checking FS_RAID.)
