4370 avoid transmitting holes during zfs send

[Christopher Siden]

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.

Chris

[Josef 'Jeff' Sipek]

Post by Christopher Siden
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.

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.

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).

dsl_dataset.c: was the ASSERT useless?

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.

Post by Christopher Siden
Chris
_______________________________________________
developer mailing list
http://lists.open-zfs.org/mailman/listinfo/developer

[Max Grossman]

Hi Jeff!

On Sun, Dec 1, 2013 at 12:02 PM, Josef 'Jeff' Sipek

Post by Josef 'Jeff' Sipek

Post by Christopher Siden
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.

Post by Josef 'Jeff' Sipek
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.

Post by Josef 'Jeff' Sipek
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.

Post by Josef 'Jeff' Sipek
dsl_dataset.c: was the ASSERT useless?

That assert is no longer valid, as the code has been moved above the check
for holes.

Post by Josef 'Jeff' Sipek
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
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

Post by Josef 'Jeff' Sipek

Post by Christopher Siden
Chris
_______________________________________________
developer mailing list
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
http://lists.open-zfs.org/mailman/listinfo/developer

[Josef 'Jeff' Sipek]

Post by Max Grossman
Hi Jeff!
On Sun, Dec 1, 2013 at 12:02 PM, Josef 'Jeff' Sipek

Post by Josef 'Jeff' Sipek

Post by Christopher Siden
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...

...

Post by Max Grossman

Post by Josef 'Jeff' Sipek
Trivial nits related to coding style (I don't know what the prefered openzfs
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 {}

I'm happy. Looks good to me. (Do whatever you feel is appropriate with the
coding style nits.)

Jeff.