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)
On Tue, Jun 10, 2014 at 10:02 PM, Xin Li <[email protected]> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> On 6/2/14, 10:27 AM, Matthew Ahrens wrote:
> > 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
> > "zpool set feature@lz4_compress=enabled"), and you do "zfs set
> > 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).
>
> This reminds me -- I have talked with Martin Matuska ([email protected])
> a while ago on this idea and eventually I have the attached patch (for
> 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-----
>
_______________________________________________
developer mailing list
[email protected]
http://lists.open-zfs.org/mailman/listinfo/developer
