Re: Review of projects/nand branch

[ Available lists | Index of freebsd-fs | Month of Apr 2012 | Week of 14 Apr 2012 | Raw email | View thread | Wrap long lines | Reply | Tag ]
From
Grzegorz Bernacki <gjb@semihalf.com>
Date
14 Apr 2012 04:13:32
Subject
Re: Review of projects/nand branch
Message-ID
4F88F966.5030300@semihalf.com


[ Hide this part ]
Hi Marcel,

Please find updated status of fixing bugs inlined.

W dniu 2012-04-03 05:10, Grzegorz Bernacki pisze:
> W dniu 2012-04-02 23:37, Marcel Moolenaar pisze:
>> Grzegorz,
>>
>> I reviewed the changes on the projects/nand branch and in general
>> it's of high quality and any problems, improvements and/or cleanups
>> can be addressed after it gets merged into -current, with the
>> following caveat:
>> 1. Changes to sys/kern, sys/geom and sys/sys should be reviewed and
>> approved by people on fs@freebsd.org and/or geom@freebsd.org. I
>> saw comments from pjd already for example.
Changes to geom has been reverted. We are working on remove rest of changes
from sys/kern and sys/sys
>>
>> 2. Please address the following points before merging onto head:
>>
>> o In include/Makefile: fs/fifofs is removed. Deliberate?
I applied incorrectly created patch. It was fixed with merge from HEAD.
>>
>> o In sbin/Makefile: we should have a distinct MK_NANDFS option
>> for use by the file system code.

- Is a separate MK_NANDFS knob really needed? Other filesystems don't seem to
follow this route
- The sys/fs/nandfs is only included per kernel config option, other userspace
components per MK_NAND
- Do you really think it is useful to have NAND framework built without NANDFS
and vice versa, the FS without userland tools for it?

>> o In sbin/nandfs/nandfs.8: could elaborate for what one could
>> use the snapshots.
Will be fixed
>> o In sbin/nandfs/nandfs.h: define NANDFS_H.
Fixed
>> o In sbin/nandfs/nandfs.c: usage() is wrong.
>> o In sbin/nandfs/Makefile: $FreeBSD$ is missing.
Fixed
>> o In sbin/mount_nandfs/mount_nandfs.8: copyright notice seems
>> bogusly copied. Also, cleanerd is gone so it needs updating.
>> o In sbin/mount_nandfs/mount_nandfs.c: cleanerd is gone, so
>> this file could do with a some cleanups.
>> o In sbin/mount_nandfs/Makefile: $FreeBSD$ is missing.
mount_nandfs have been removed.
>> o In sbin/mount/mntopts.h: cleanerd is gone, so should not be
>> needed anymore.
Fixed
>> o In sbin/newfs_nandfs/newfs_nandfs.c: we have CRC32 code for
>> re-use. No need to implement again.
Will be fixed later.
>>
>> o In sbin/newfs_nandfs/Makefile: missing DPADD.
Fixed
>>
>> o In share/mk/bsd.own.mk: Add NANDFS as well. May also want to
>> add NANDSIM separately.
>> o In share/man/man5/Makefile: should be NANDFS.
Both above will be fixed soon.
>>
>> o In usr.sbin/nandtool/Makefile: missing $FreeBSD$
>> o In usr.sbin/nandsim/Makefile: missing $FreeBSD$
Both above are fixed
>> o usr.sbin/Makefile should have nandtool and nandsim when
>> MK_NAND is defined.
>> o In lib/Makefile: should be MK_NANDFS; not MK_NAND.
>> o In lib/libstand/nandfs.c: should use common CRC32 impl.
>> o In lib/libstand/Makefile: should be MK_NANDFS; not MK_NAND.
>> o Please get buy-in for changes to sys/kern/vfs_vnops.c,
>> sys/kern/vfs_bio.c and sys/kern/vfs_subr.c from people
>> on fs@freebsd.org.
>> o In sys/modules/Makefile: always build nandfs module. Make
>> nandsim module dependent on MK_NAND or MK_NANDSIM if added.
All above will be fixed soon.
>>
>> o Please get buy-in for changes to sys/geom/geom_dev.c,
>> sys/geom/geom_disk.c, sys/geom/geom_disk.h, sys/geom/geom.h
>> and sys/geom/geom_slice.c from people on geom@freebsd.org.
Geom changes has been removed.
>>
>> o Please get buy-in for changes to sys/sys/disk.h and
>> sys/sys/bio.h from people on either fs@freebsd.org or
>> geom@freebsd.org.
Those changes has been removed.
>>
>> I also have a general usability question relating snapshots.
>> Currently snapshots are read-only. A useful feature in the
>> embedded space is to make a snapshot, attempt a software
>> update and revert to the snapshot if and when the update fails
>> or gets aborted. Is it possible to extend the snapshot feature
>> in the future to allow for this use case (i.e. ignore any and
>> all modifications that happened after a snapshot was made and
>> mount the snapshot R/W as representing the current/latest state
>> of the file system)?
We are working on this.

thanks,
Grzesiek

Elapsed time: 0.203 seconds