Matthew Ahrens via illumos-zfs
2014-06-11 05:45:33 UTC
This change looks reasonable. I think we additionally need to:
- To deal with pools that already have the feature enabled but not active,
activate the feature if it is enabled but not yet active.
spa_sync_upgrades() would be an appropriate place to do this.
- Don't bother activating the feature when doing "zfs set compression=lz4"
(i.e. remove the ZFS_PROP_COMPRESSION case from zfs_prop_set_special();
remove zfs_prop_activate_feature{_sync,_check})
- Update the zpool-features.5 manpage to indicate that the feature will be
activated when enabled. See the documentation of the birth_hole feature
for example wording to copy.
also:
zio_compress.c:
+ if (method == ZIO_COMPRESS_ON_VALUE) {
I think you want
+ if (method == ZIO_COMPRESS_LZ4) {
because this is specifically dealing with LZ4. Otherwise if the ON_VALUE
changes, this block would be incorrect.
--matt
(cc'd illumos lists as this topic came up there today as well with a
different patch submitted)
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com
- To deal with pools that already have the feature enabled but not active,
activate the feature if it is enabled but not yet active.
spa_sync_upgrades() would be an appropriate place to do this.
- Don't bother activating the feature when doing "zfs set compression=lz4"
(i.e. remove the ZFS_PROP_COMPRESSION case from zfs_prop_set_special();
remove zfs_prop_activate_feature{_sync,_check})
- Update the zpool-features.5 manpage to indicate that the feature will be
activated when enabled. See the documentation of the birth_hole feature
for example wording to copy.
also:
zio_compress.c:
+ if (method == ZIO_COMPRESS_ON_VALUE) {
I think you want
+ if (method == ZIO_COMPRESS_LZ4) {
because this is specifically dealing with LZ4. Otherwise if the ON_VALUE
changes, this block would be incorrect.
--matt
(cc'd illumos lists as this topic came up there today as well with a
different patch submitted)
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
FreeBSD, I have personally running it on my own storage system for
quite a while now).
I was relented to push it for discussion because I don't have any
scientific testing data and don't have free cycles to test it against
some corner cases (e.g. running with v28 pools, etc.).
It also uses LZ4 for metadata when the feature is active, by the way.
I took a different approach than Daniil's version but other than not
activating it, the result should be the same.
(Unscientific data from my storage system shows a clear win for using
LZ4 to compress metadata, though).
Cheers,
-----BEGIN PGP SIGNATURE-----
iQIcBAEBCgAGBQJTl+L6AAoJEJW2GBstM+nsJ0wP/3S58WnJ5muhMcClLNNaoZGK
Z/CsXRVm7qTylMFB9fHhlMgb1IsAKXdyiz82RGa6LWuAQecrQksaRV40Tb8ntGU2
EttTmXoXWDF2zGbyoWyHnxne/ODc3rhfWoNGKzrElGnoARlGfA9Eri9Kr5u0xGI8
e9R/flitCz8duzXQluLjl+DYRAWUw2Tx+MyoFhOtqkGBOf6QmrrYTg2eXqN28n4s
EW55kqb3825adY7WuOZeIYa4AlK5owCqooOgnELaLSFPFcGFwLvx8rQmJ6UdKdlv
h0lfATfK7A9NOglRpJ3PafVz+cpZCoI4plz6I5kdeShK0a9P0INgVbis2nEv7aZS
Hn/rI1l4zRF7ssv+Ke9aAunSpRrgFoZvdP+BrBmvrBdc8M2WMzKbxmpJpH0B0u7q
acXU6L469nD0U/oCHAnAW1Usw2ZCBrzHWVzbpwlbk3hGg9TqNiNcRfcmDAYefZzB
PMIijLX4YSI9tyW32zEuyz73LkN/ZCuWcAuf5m9Cpk6zHBPeJ5z7HHX+arVHlmkf
pZRqa0ON9rdXsuvOt6BGNUmk1uKP5/7ftbIbFnCuuWufcBbo5QOQas83lGZ4cr8p
FXatM6fcQFNgAya++x3FimYWb7qw73jvpPIHhi6GYzfGRErFr9UZKZg74n4IlPgs
VnajGHXxo6FIHJL+1AU2
=d+4k
-----END PGP SIGNATURE-----
-------------------------------------------Hash: SHA512
We've seen that overall, LZ4 is a better compression algorithm
than LZJB. It compresses and decompresses faster, and gets a
better compression ratio. It's been in use long enough for us to
shake out the few initial problems with the implementation.
I think we should change the default compression algorithm in ZFS
to LZ4. Specifically, if the LZ4 feature is enabled (e.g. via
compression=on", then it will compress with LZ4 rather than LZJB..
We should probably also compress metadata with LZ4 if the feature
is enabled (after verifying that indirect blocks compress at least
as well as LZ4).
I am pretty busy with other ZFS work at the moment, but I'd be
happy to guide / mentor someone from the community who would like
to implement this. It would also make a good hackathon project at
the OpenZFS Developer Summit
<http://www.open-zfs.org/wiki/OpenZFS_Developer_Summit_2014>
(November 2014 in San Francisco).
a while ago on this idea and eventually I have the attached patch (forthan LZJB. It compresses and decompresses faster, and gets a
better compression ratio. It's been in use long enough for us to
shake out the few initial problems with the implementation.
I think we should change the default compression algorithm in ZFS
to LZ4. Specifically, if the LZ4 feature is enabled (e.g. via
compression=on", then it will compress with LZ4 rather than LZJB..
We should probably also compress metadata with LZ4 if the feature
is enabled (after verifying that indirect blocks compress at least
as well as LZ4).
I am pretty busy with other ZFS work at the moment, but I'd be
happy to guide / mentor someone from the community who would like
to implement this. It would also make a good hackathon project at
the OpenZFS Developer Summit
<http://www.open-zfs.org/wiki/OpenZFS_Developer_Summit_2014>
(November 2014 in San Francisco).
FreeBSD, I have personally running it on my own storage system for
quite a while now).
I was relented to push it for discussion because I don't have any
scientific testing data and don't have free cycles to test it against
some corner cases (e.g. running with v28 pools, etc.).
It also uses LZ4 for metadata when the feature is active, by the way.
I took a different approach than Daniil's version but other than not
activating it, the result should be the same.
(Unscientific data from my storage system shows a clear win for using
LZ4 to compress metadata, though).
Cheers,
-----BEGIN PGP SIGNATURE-----
iQIcBAEBCgAGBQJTl+L6AAoJEJW2GBstM+nsJ0wP/3S58WnJ5muhMcClLNNaoZGK
Z/CsXRVm7qTylMFB9fHhlMgb1IsAKXdyiz82RGa6LWuAQecrQksaRV40Tb8ntGU2
EttTmXoXWDF2zGbyoWyHnxne/ODc3rhfWoNGKzrElGnoARlGfA9Eri9Kr5u0xGI8
e9R/flitCz8duzXQluLjl+DYRAWUw2Tx+MyoFhOtqkGBOf6QmrrYTg2eXqN28n4s
EW55kqb3825adY7WuOZeIYa4AlK5owCqooOgnELaLSFPFcGFwLvx8rQmJ6UdKdlv
h0lfATfK7A9NOglRpJ3PafVz+cpZCoI4plz6I5kdeShK0a9P0INgVbis2nEv7aZS
Hn/rI1l4zRF7ssv+Ke9aAunSpRrgFoZvdP+BrBmvrBdc8M2WMzKbxmpJpH0B0u7q
acXU6L469nD0U/oCHAnAW1Usw2ZCBrzHWVzbpwlbk3hGg9TqNiNcRfcmDAYefZzB
PMIijLX4YSI9tyW32zEuyz73LkN/ZCuWcAuf5m9Cpk6zHBPeJ5z7HHX+arVHlmkf
pZRqa0ON9rdXsuvOt6BGNUmk1uKP5/7ftbIbFnCuuWufcBbo5QOQas83lGZ4cr8p
FXatM6fcQFNgAya++x3FimYWb7qw73jvpPIHhi6GYzfGRErFr9UZKZg74n4IlPgs
VnajGHXxo6FIHJL+1AU2
=d+4k
-----END PGP SIGNATURE-----
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com