Skip site navigation (1)Skip section navigation (2)
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>