> On June 7, 2015, 8:50 p.m., Josef 'Jeff' Sipek wrote: > > Looks like at the very least libmd's mapfile will need more care. Pretty > > sure those new symbols should be in a new SYMBOL_VERSION section. > > > > The crypto tests added to zfs-tests...they seem unrelated to zfs. > > > > skien_iv.h seems to include a bunch of const arrays. Shouldn't these be in > > a .c file instead? > > > > I think it'd be nice to separate out some of the not directly related > > changes (e.g., compile time asserts). I'd still like them to get > > committed, but ZFS commits are complex enough without all the extra > > interfaces/non-ZFS features they introduce. > > > > edonr_byteorder.h: what's IDI,NTNU? > > > > system-header.mf: no usr/include/edonr.h? Alternatively, do we really need > > usr/include/skein.h? > > > > edonr info in the reviewboard description: Why is compression required for > > nopwrite+edonr? > > Matthew Ahrens wrote: > Thanks for the feedback. FYI: you can click on a line in the review and > add a comment specific to that location. This makes it easier to track and > respond to each point separately. > > mapfile: will do. > > tests: I'll check with John Kennedy and see if there's a better way. > > skein_iv.h: I agree; it seems like this should simply be a .c file > itself. Is there any concern about deviating from "upstream" here? > > Separating out: I'm not sure when I'll have time to do that. Let me know > if it's a blocker for you. > > edonr_byteorder.h: NTNU is the original author's domain name, a Norwegian > university. Don't know what IDI means. > > Compression is required for nopwrite in general, because if compression > is disabled we want a simple space usage semantic: if you write a logical > block, and don't take any snapshots, then you can overwrite it without > running out of space. Compression breaks this semantic but the user > explictly requested it. We use it to enable other low/zero cost space > savings as well, like blocks of all zero turn into holes.
I was actually looking at a unified diff. I can't stand reviewboard's lagginess even with smaller changes. This is far from a small change and I just don't have the patience to wait for minutes for things to load. :) In general, I'd be worried about diverging from upstream. However, I *strongly* suspect that something like digest algos are likely to not be updated all the time by upstream so I'm inclined to do things "right". I'd really appreciate if you split it up. The mega-commits that seemed to be the norm at Sun are a pain to reason about, they introduce dependencies between commits that are otherwise unrelated (making selective backporting a PITA), etc. NTNU,IDI - ok, just making sure it wasn't random keyboard noise introduced by us. Interesting...I didn't realize that compression changed the mental model of how things worked (aside from the obvious). Thanks for the explanation. - Josef 'Jeff' ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/223/#review744 ----------------------------------------------------------- On June 7, 2015, 11:42 a.m., Matthew Ahrens wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.csiden.org/r/223/ > ----------------------------------------------------------- > > (Updated June 7, 2015, 11:42 a.m.) > > > Review request for OpenZFS Developer Mailing List and Saso Kiselkov. > > > Bugs: 4185 > https://www.illumos.org/projects/illumos-gate//issues/4185 > > > Repository: illumos-gate > > > Description > ------- > > 4185 add new cryptographic checksums to ZFS: SHA-512, Skein, Edon-R > > Note that this allocates new checksum algorithm values. If you have > developed new checksum algorithms on a private branch, now would be a good > time to speak up. > > enum zio_checksum { > ... > ZIO_CHECKSUM_NOPARITY, // == 10, last one in illumos > ZIO_CHECKSUM_SHA512, // == 11, proposed addition > ZIO_CHECKSUM_SKEIN, // == 12, proposed addition > ZIO_CHECKSUM_EDONR, // == 13, proposed addition > } > > Note that this doesn't change any defaults, e.g. "zfs set dedup=on" still > changes the checksum algorithm to sha256. You need to explicitly opt in to > use the new checksum algorithms. We'll probably eventually change that > default for dedup=on to sha512. (This is the same strategy we used with > checksum=on and lz4.) > > Summary from the zpool-features manpage: > > sha512 > > This feature enables the use of the SHA-512/256 truncated hash > algorithm (FIPS 180-4) for checksum and dedup. The native 64-bit > arithemtic of SHA-512 provides an approximate 50% performance boost > over SHA-256 on 64-bit hardware and is thus a good minimum-change > replacement candidate for systems where hash performance is > important, but these systems cannot for whatever reason utilize the > faster skein and edonr algorithms. > > Booting off of pools utilizing SHA-512/256 is supported (provided > that the updated GRUB stage2 module is installed). > > skein > > This feature enables the use of the Skein hash algorithm for > checksum and dedup. Skein is a high-performance secure hash > algorithm that was a finalist in the NIST SHA-3 competition. It > provides a very high security margin and high performance on 64-bit > hardware (80% faster than SHA-256). This implementation also > utilizes the new salted checksumming functionality in ZFS, which > means that the checksum is pre-seeded with a secret 256-bit random > key (stored on the pool) before being fed the data block to be > checksummed. Thus the produced checksums are unique to a given > pool, preventing hash collision attacks on systems with dedup. > > edonr > > This feature enables the use of the Edon-R hash algorithm for > checksum, including for nopwrite (if compression is also enabled, > an overwrite of a block whose checksum matches the data being > written will be ignored). In an abundance of caution, Edon-R can > not be used with dedup (without verification). > > Edon-R is a very high-performance hash algorithm that was part of > the NIST SHA-3 competition. It provides extremely high hash > performance (over 350% faster than SHA-256), but was not selected > because of its unsuitability as a general purpose secure hash > algorithm. This implementation utilizes the new salted > checksumming functionality in ZFS, which means that the checksum is > pre-seeded with a secret 256-bit random key (stored on the pool) > before being fed the data block to be checksummed. Thus the > produced checksums are unique to a given pool, blocking hash > collision attacks on systems with dedup. > > Original author: Matthew Ahrens & Saso Kiselkov > > > Diffs > ----- > > usr/src/test/zfs-tests/tests/functional/checksum/edonr/i386/Makefile > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/edonr/amd64/Makefile > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/edonr/edonr_test.c > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/edonr/Makefile > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/run_skein_test.ksh > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/run_sha2_test.ksh > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/run_edonr_test.ksh > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/Makefile PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/Makefile.subdirs > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/Makefile > 65838aba43852c2c1e376675d4f45fabebc59433 > usr/src/test/zfs-tests/runfiles/openindiana.run > 8908515500bb649ae123090d4a32fc8f587f7408 > usr/src/test/zfs-tests/runfiles/omnios.run > 8908515500bb649ae123090d4a32fc8f587f7408 > usr/src/test/zfs-tests/runfiles/delphix.run > c98ed9f88885ef6763cab58371f485078a0eb556 > usr/src/test/zfs-tests/include/libtest.shlib > 7c25516d5171e1d48d29207c4a0c7f0c3a9c2e45 > usr/src/pkg/manifests/system-test-zfstest.mf > dab3f35c906b2295eb505a3fa3477fa208cb66cc > usr/src/pkg/manifests/system-kernel.mf > 4832229cb2f292e65937c01214b1bba231e91113 > usr/src/pkg/manifests/system-header.mf > eba786e9d07490c330eabeadcaf82eee9b365222 > usr/src/man/man5/zpool-features.5 fbefb37b80914566739620d0201183f0243e9306 > usr/src/man/man1m/zfs.1m 752074bd4fe384512cc85262572e8837c15c17ca > usr/src/lib/libzfs/common/libzfs_dataset.c > e491a65c9396fff26b5b786f3a234f076a2e6a08 > usr/src/lib/libmd/sparcv9/Makefile 5dd3f2eac36c5381fe651a2a795eb1be48d28602 > usr/src/lib/libmd/sparc/Makefile 5f491cc9af95549b865cd076b12c3870bbd68c95 > usr/src/lib/libmd/i386/Makefile 3e6fe00e17372e0666088927aec0e25ce97af556 > usr/src/lib/libmd/common/skein.h PRE-CREATION > usr/src/lib/libmd/common/mapfile-vers > a70144e833e3c609b4c86c35899a4df513c5e0b2 > usr/src/lib/libmd/amd64/Makefile 3872749fbbfdbde48784cd1146dee0fe52fba043 > usr/src/lib/libmd/inc.flg 2652430b841348a4ad25ab990cf7e793ce3e5e7e > usr/src/lib/libmd/Makefile.com af8cac390bca3421c0f9535c8e9ebf18a75a51e8 > usr/src/grub/grub-0.97/stage2/zfs-include/zio.h > 3b893f451ef0d6d882e477860d816c6f287ad172 > usr/src/grub/grub-0.97/stage2/fsys_zfs.h > 2f77a5b6a71a984e2c8b56a5a9e62ab8472e9340 > usr/src/common/zfs/zfs_prop.c e145b1c866e32ab21be324b8ebc717f1571a104a > usr/src/common/zfs/zfeature_common.c > 3358e5eb87bada47b77d4060b88a614e116d9b7a > usr/src/common/crypto/skein/skein_port.h PRE-CREATION > usr/src/common/crypto/skein/skein_impl.h PRE-CREATION > usr/src/common/crypto/skein/skein.c PRE-CREATION > usr/src/common/crypto/skein/THIRDPARTYLICENSE.descrip PRE-CREATION > usr/src/common/crypto/edonr/edonr_byteorder.h PRE-CREATION > usr/src/common/crypto/edonr/edonr.c PRE-CREATION > usr/src/uts/sparc/skein/Makefile PRE-CREATION > usr/src/uts/intel/zfs/Makefile be0f82115c88339e1fb8e0f7d96f5d5a7f917450 > usr/src/uts/intel/edonr/Makefile PRE-CREATION > usr/src/uts/common/fs/zfs/sys/zio_checksum.h > a921a2f1396ed720d563a0e16822283276495f0f > usr/src/uts/common/fs/zfs/sys/spa_impl.h > 56e6adb713d201ade02ca665008e806c88b66dd3 > usr/src/uts/common/fs/zfs/zio.c aa0f2945dd948158a1ea8a0f72558b7f892a1017 > usr/src/uts/common/fs/zfs/spa_misc.c > c5af055a8608745ec5099a4f4ebf4a159767d0b4 > usr/src/uts/common/fs/zfs/skein_zfs.c PRE-CREATION > usr/src/uts/common/fs/zfs/dsl_dataset.c > a11926b58f1a6741b53216509ad00df37d0a1ac8 > usr/src/uts/common/fs/zfs/dmu.c 43bcfee91cd1308447ba739449355a0d7730441c > usr/src/uts/common/fs/zfs/arc.c 09d2e1dd8fdf6648d863d4c036c16b3f7981dbf5 > usr/src/uts/common/crypto/io/edonr_mod.c PRE-CREATION > usr/src/uts/common/Makefile.files ca912c05dd5160262ca9a55c8672e70e09269760 > usr/src/test/zfs-tests/tests/functional/checksum/skein/sparcv9/Makefile > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/skein/sparc/Makefile > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/skein/i386/Makefile > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/skein/amd64/Makefile > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/skein/skein_test.c > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/skein/Makefile > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/sha2/sparcv9/Makefile > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/sha2/sparc/Makefile > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/sha2/i386/Makefile > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/sha2/amd64/Makefile > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/sha2/sha2_test.c > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/sha2/Makefile PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/edonr/sparcv9/Makefile > PRE-CREATION > usr/src/test/zfs-tests/tests/functional/checksum/edonr/sparc/Makefile > PRE-CREATION > usr/src/uts/sparc/zfs/Makefile b891e4fae1f2e2c8505dfa96f0097580b146299e > usr/src/uts/sparc/edonr/Makefile PRE-CREATION > usr/src/uts/sparc/Makefile.sparc e0913ce81fa9e9e28c5b9022a87210aeaf4f3add > usr/src/uts/intel/skein/Makefile PRE-CREATION > usr/src/uts/intel/Makefile.intel bd346b5dab4be7be27e14ada63405843eaa6a885 > usr/src/uts/common/sys/skein.h PRE-CREATION > usr/src/uts/common/sys/sha2.h ad46dd683ad54b0169130a70856d879c7840cc57 > usr/src/uts/common/sys/edonr.h PRE-CREATION > usr/src/uts/common/sys/debug.h 27b84beb88071639fca78fca30cbba305dd3c4ac > usr/src/uts/common/sys/crypto/common.h > 8fc7b05de6289f87de1670bf0a6def5b1e84f423 > usr/src/uts/common/sys/Makefile c99ef8accbb80c34d48393d0d057ebc57ad85d78 > usr/src/uts/common/fs/zfs/zio_checksum.c > d1c60c3ffaba0521477f3b0709e079d33bb8a9cc > usr/src/uts/common/fs/zfs/zfs_ioctl.c > 1826c8aaacdf7acdc5ec66cab491407ef2f914cf > usr/src/uts/common/fs/zfs/sys/zio.h > 198dc92387092fdbcbf2716da21444b9bdde1bec > usr/src/uts/common/fs/zfs/sys/spa.h > 2c4887b6ca5040d39b46c6f6be531607946a5e10 > usr/src/uts/common/fs/zfs/sys/dmu.h > 9864129e18c0d6ed5ef3b4ec05f8b63628736ed3 > usr/src/uts/common/fs/zfs/spa.c e9d2432a81791e75ad2f8b41fe99c9da9d345454 > usr/src/uts/common/fs/zfs/sha256.c f515be6bb30425ed89ccb070bb913337c886c71c > usr/src/uts/common/fs/zfs/edonr_zfs.c PRE-CREATION > usr/src/uts/common/fs/zfs/dmu_send.c > 7d728c6877e58c0452ad0e4e7ea0eedc9dfbbaf7 > usr/src/uts/common/fs/zfs/ddt.c 7d938965722a143af64d287de9bfdd6e5c8d922c > usr/src/uts/common/crypto/io/skein_mod.c PRE-CREATION > usr/src/uts/common/Makefile.rules c5e728fdffcb35ad73813a5ee2c3f0050b4fb92f > usr/src/lib/libmd/Makefile.targ 58ec7b30e50d235741b59e2cf9a76300976ada7d > usr/src/lib/libmd/Makefile ddd89d7382bf7d393536904088790d2204ccf329 > usr/src/grub/grub-0.97/stage2/zfs_sha256.c > 393eaee05b42e535de8eb6041f19c377bdc15478 > usr/src/grub/grub-0.97/stage2/fsys_zfs.c > 91b4bcec6fb4119c462f71dffb6b16475e5f4180 > usr/src/grub/capability 111cd61ccb550faf17842be4bf3dadd01dc5f1ae > usr/src/common/zfs/zfs_fletcher.c fa43ce6bdb5dd1d625896fe73d0cbd84d2631d69 > usr/src/common/zfs/zfs_fletcher.h b49df0cf4f0fd2d613d9da1a1d93a68ab19dbb1a > usr/src/common/zfs/zfeature_common.h > 3f54b392be4248ea059a9d0b06ee1df756495d34 > usr/src/common/crypto/skein/skein_iv.h PRE-CREATION > usr/src/common/crypto/skein/skein_block.c PRE-CREATION > usr/src/common/crypto/skein/THIRDPARTYLICENSE PRE-CREATION > usr/src/common/crypto/sha2/sha2.c 1a2603ccf00cf933e20d75f427fa4d2e6c63551b > > Diff: https://reviews.csiden.org/r/223/diff/ > > > Testing > ------- > > ztest, zfs test suite > (note that ztest uses the new algorithms) > > http://jenkins.delphix.com/job/zfs-precommit/2447/ > > > Thanks, > > Matthew Ahrens > >
_______________________________________________ developer mailing list developer@open-zfs.org http://lists.open-zfs.org/mailman/listinfo/developer
