Re: patch: MFSv3 support (libsa) for boot2 (i386)

[Evgeniy Ivanov <lolkaantimat%gmail.com@localhost>]

Hi,

On Wed, Dec 28, 2011 at 6:35 PM, Izumi Tsutsui 
<tsutsui%ceres.dti.ne.jp@localhost> wrote:
> Evgeniy Ivanov wrote:
>
>> I was trying to add bootxx_minifs3 similar to the bootxx_ext2fs. I
>> failed to figure out how to use bootxx_ext2fs, because UFS and FAT are
>> the only filesystems hardcoded in bootxx (pbr.S). Also it seems that a
>> small ext2 boot partition is not an option for i386, because bootxx
>> and boot must be on the same partition (and installing bootxx on ext2
>> kills FS, because 1Kb < ~8Kb). Though commit comment says it can be
>> done. Could you please explain how to use bootxx_ext2fs?
>
> With the following disklabel:
> ---
> 16 partitions:
> #        size    offset     fstype [fsize bsize cpg/sgs]
>  a:   1833457       128 Linux Ext2   1024  8192        # (Cyl.      0*-   
> 1819*)
>  b:    263537   1833615       swap                     # (Cyl.   1819*-   
> 2080*)
>  c:   2097120        32     unused      0     0        # (Cyl.      0*-   
> 2080*)
>  d:   2097152         0     unused      0     0        # (Cyl.      0 -   
> 2080*)
>  e:        96        32       boot                     # (Cyl.      0*-     
>  0*)
> ---
> i.e. allocate a small explicit "boot" partition before root (a:) partition.
>
> installboot against /dev/wd0e, then bootxx_ext2fs is written
> at top of the NetBSD partition and it loads /boot from wd0a.

Aha, it's a real disklabel, not fictitious one, and 'a' works as a
fallback partition, right? With MBR it will not work, will it?
Also 'e' is 0 - 0 and 'a' is 0 - 1819, so they overlap. Why 'a' is not
overwritten by bootxx then? Sorry, I'm not familiar with disklabels.

>> > - I don't know about MINIX FS v3, but it looks similar to Linux Ext2fs.
>> >  Is it difficult to share some code among them?
>>
>> It's also very similar to UFS (seems like Ext2fs is based on that
>> code). There're some functions which have much in common, but
>> different either in structure members names (like xxx_stat) or in very
>> small details. I think sharing such small pieces of code will make
>> things worse. Probably that's the reason why (IIRC) in Linux ext2,
>> ext3 and ext4 do not share common code.
>
> Ok, I see.
>
>> > - Do you have any plan to add kernel support for the MINIX FS v3?
>>
>> No.
>
> Ok, but in that case someone might claim MINIX FS should be disabled
> by default in x86 /boot.
> (to avoild heap overflow etc. as ext2fs was disabled once before)

I'll add flags to disable by default.

>> >  If not, are changes outside sys/libsa necessary?
>> >  (common/lib/libutil, libkern/xlat_mbr_fstype.c, sys/disk.h etc.)
>>
>> It's required by biosdisk modification. Biosdisk itself requires just
>> one of those, but system build fails without other modifications
>> (switch doesn't list everything, but adding a proper branch requires
>> some another changes outside). Also it's nice to show to disklabel
>> users name of MFS instead of "Unknown".
>
> Ok, but probably we need some approval (even by silence)
> because it's exported API to userland.
> (i.e. we won't be able to rename it for compatibility once it's exported)

OK.

>> > - What environment do you test these loaders?
>> >  Is there any tool to create/check these file system like
>> >  e2fsprogs for Ext2fs?
>>
>> To create MFS (sub)partitions and to format I use part and mkfs.mfs
>> respectively. IIRC it's available in MINIX 3 only, but there is a life
>> cd.
>> To check MFS implementation I've inserted a test into boot2: it reads
>> 3 files and calculates md5 sums. The biggest file had all levels of
>> indirection. Ls was used to check if reads directories correctly.
>> To multiboot I use an example multiboot kernel from grub's multiboot
>> specification.
>>
>> I use VMWare and can share my test virtual disk.
>
> Ok. It's always good thing "what are tested or not in the patch"
> in reports or logs :-)
>
>> > - There are some "XXX should handle LARGEFILE" comments (that I put
>> >  for ext2fs REV1 inode), but does MINIX FS have the similar member?
>> >  It doesn't seem there are extra members in struct mfs_dinode.
>>
>> Removed odd comments.
>
> BTW, can MINIFS FS handle >2GB files or not?

It can't.

>> > - struct mfs_sblock seems to have many uint16_t and one char members.
>> >  Probably it's better to put explicit padding and specify __packed
>> >  to avoid unexpected machine dependent alignment restrictions.
>>
>> It is the last member in ondisk structure, so machine dependent
>> alignment after is just fine.
>
> Ah, ok.
>
>> > - in sys/lib/libsa/mfsv3.c:mfsv3_i_bswap()
>> >  - mdi_zone[] in  is unused? (untesed on big endian?)
>>
>> Sorry, it's a pasto. I didn't test on big endian.
>
> It's better to check at least it (at least libsa) compiles
> on build environments for big endian machines.

I'll build cross tools and try compiling.

>> >  - zone_t is typedef'ed as uint32_t. Shouldn't mdi_zone[] members
>> >   also be swapped?
>>
>> Oh, they're swapped in a loop at the end of mfsv3_i_bswap(). That
>> local mdi_zone[] didn't affect anything.
>
>  :
>>> +    for (i = 0; i < V2_NR_TZONES; i++)
>>> +            new->mdi_zone[i] = (zone_t) old->mdi_zone[i];
>
> Not a cast, but should be bswap32()?

Sorry for confusion. Like in ext2fs swapping occurs in block_map().

>> > + * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND 
>> > CONTRIBUTORS
>> >
>> > Probably not TNF, but the author or copyright holders.
>>
>> Fixed.
>
>  :
>>> + * THIS SOFTWARE IS PROVIDED BY THE VRIJE UNIVERSITEIT AND CONTRIBUTORS 
>>> ``AS
>
> For redistributors less variants of disclaimer make things easier,
> so how about "COPYRIHGT HOLDERS" like src/sys/dev/ic/bwi.c:
>
>>>  * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS

OK.

>
>> Do I need to send-pr?
>
> Yes, so that we can search by keywords (digging ML archive is a bit annoying)
> and we can also note PR number in commit log :-)

Alright :-) Then I do some modifications, test email and send-pr new patches.



-- 
Evgeniy