Date: Mon, 2 Apr 2012 14:37:18 -0700 From: Marcel Moolenaar <marcel@xcllnt.net> To: Grzegorz Bernacki <gjb@semihalf.com> Cc: geom@FreeBSD.org, fs@FreeBSD.org Subject: Review of projects/nand branch Message-ID: <EE2F4D7D-F6D9-48D4-923A-99DAA62698B2@xcllnt.net>
next in thread | raw e-mail | index | archive | help
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.
2. Please address the following points before merging onto head:
o In include/Makefile: fs/fifofs is removed. Deliberate?
o In sbin/Makefile: we should have a distinct MK_NANDFS option
for use by the file system code.
o In sbin/nandfs/nandfs.8: could elaborate for what one could
use the snapshots.
o In sbin/nandfs/nandfs.h: define NANDFS_H.
o In sbin/nandfs/nandfs.c: usage() is wrong.
o In sbin/nandfs/Makefile: $FreeBSD$ is missing.
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.
o In sbin/mount/mntopts.h: cleanerd is gone, so should not be
needed anymore.
o In sbin/newfs_nandfs/newfs_nandfs.c: we have CRC32 code for
re-use. No need to implement again.
o In sbin/newfs_nandfs/Makefile: missing DPADD.
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.
o In usr.sbin/nandtool/Makefile: missing $FreeBSD$
o In usr.sbin/nandsim/Makefile: missing $FreeBSD$
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.
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.
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.
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)?
I'd like to thank Semihalf and the Foundation for bringing
this code into FreeBSD. Well done!
Thanks,
--
Marcel Moolenaar
marcel@xcllnt.net
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?EE2F4D7D-F6D9-48D4-923A-99DAA62698B2>