Hi Jeff! On Sun, Dec 1, 2013 at 12:02 PM, Josef 'Jeff' Sipek <[email protected]>wrote:
> On Sun, Dec 01, 2013 at 10:55:16AM -0800, Christopher Siden wrote: > > http://code.delphix.com/illumos-4370/index.html > > > > Work by Max Grossman. See bug and updates to zpool-features(5) manpage > > in diff for details. > > Keep in mind that I'm not very familiar with the code... with that said, > the > feature refcount cache isn't mentioned in either bug. Is it significant to > get mentioned? Does it deserve a separate bug id? Similarly, the > enabled_txg & hole_birth features aren't mentioned in the bugs. > The feature refcount cache is simply an optimization implemented as a part of the hole birth feature. Otherwise, we'd go out to disk to check if the hole birth feature is enabled every time a block is freed. I don't think this requires a separate bug, and there's some explanation of the cache in feature_get_refcount and spa_load_impl. Similarly, the enabled_txg and hole_birth features are implementation details of avoiding transmission of holes which are documented in the man pages. > zfs_znode.c: ASSERT0() got replaced with VERIFY0(). Do we really want to > panic when the return is non-zero on non-debug builds? I suppose, this one > has been explicitly listed in the bug. > Yes we do. > > zvol.c: the check used to return on NULL bp. Now, a NULL bp will panic the > box. (Are we guaranteed a non-null bp?) There were other instances of != > NULL check removed (dmu_send.c, dmu_diff.c, bptree.c, zdb.c). > Yes, bp is guaranteed to be non-null now. In the existing code, bp is only == NULL when the callback (in this case zvol_map_block) is called on a hole. The new code always passes the block pointer in now, regardless of whether it is a hole or not. > > dsl_dataset.c: was the ASSERT useless? > That assert is no longer valid, as the code has been moved above the check for holes. > > zfeature_common.c: zfeature_depends_on is missing a newline before { > > > Trivial nits related to coding style (I don't know what the prefered > openzfs > way is so I'm pointing out inconsistencies): > > zfeature.c: feature_get_enabled_txg() has useless {} in the if-statement > > spa_misc.c: the for-loop in spa_add() has useless {} > > dbuf.c: dbuf_write_ready() has useless {} near line 2491 > > zhack.c: line 280 has useless {} > > Jeff. > Thanks, Max > > > Chris > > _______________________________________________ > > developer mailing list > > [email protected] > > http://lists.open-zfs.org/mailman/listinfo/developer > > -- > Mankind invented the atomic bomb, but no mouse would ever construct a > mousetrap. > - Albert Einstein > _______________________________________________ > developer mailing list > [email protected] > http://lists.open-zfs.org/mailman/listinfo/developer >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
