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

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

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.

> > - 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)

> >  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)

> > - 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?

> > - 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.

> >  - 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()?

> > + * 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


> 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 :-)

---
Izumi Tsutsui