Changes look good to me. I would just request that you add a comment in spa_sync_upgrades() explaining that the LZ4 feature wasn't always activate_on_enable, so it could be enabled but not yet active. Otherwise someone might come along this code and think it is not necessary because LZ4 is activate_on_enable.
Once that is done, feel free to file an RTI and count me as a reviewer. --matt On Wed, Jun 11, 2014 at 5:56 AM, Daniil Lunev <[email protected]> wrote: > Updated patch to the LZ4 metadata compression > http://cr.illumos.org/~webrev/DKOIman/LZ4_MD/ > ZDB output is attached. Not so big gain on compression rate, though, it is > better. Also, it was shown in initial lz4 commit that lz4 is faster. That's > why changing metadata compression to the lz4 aglorithm is a reasonable idea. > --Daniil Lunev. > > > On Wed, Jun 11, 2014 at 9:46 AM, Matthew Ahrens <[email protected]> > wrote: > >> resend to [email protected], where an alternate patch was posted. >> >> --matt >> >> >> On Tue, Jun 10, 2014 at 7:23 PM, Matthew Ahrens <[email protected]> >> wrote: >> >>> This looks generally good, but I'm concerned about enabling the feature >>> from this code path. I think it will work, but it doesn't really have the >>> right locks to ensure that it will continue to work. I think >>> dmu_write_policy() should just check if the property is already active. We >>> could add a separate routine to ensure that lz4 is always active. >>> Specifically: >>> >>> 1. Register the LZ4_COMPRESS feature with >>> zfeature_register(activate_on_enable==B_TRUE). >>> 2. 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. >>> 3. 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}) >>> 4. 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. >>> >>> --matt >>> >>> >>> On Tue, Jun 10, 2014 at 12:34 PM, Daniil Lunev via illumos-developer < >>> [email protected]> wrote: >>> >>>> Hello, all. >>>> As the lz4 algorithm is better than other we have in illumos. Though, >>>> we still use lzjb to compress metadata. Here is the patch to use lz4 for >>>> the metadata when the corresponing feature is enabled. >>>> http://cr.illumos.org/~webrev/DKOIman/LZ4_MD/ Mail_msg is attached, >>>> Thanks, >>>> Daniil Lunev >>>> P.S. Sorry if it's duplicate - somehow first mail didn't go. >>>> *illumos-developer* | Archives >>>> <https://www.listbox.com/member/archive/182179/=now> >>>> <https://www.listbox.com/member/archive/rss/182179/21175174-cd73734d> >>>> | Modify >>>> <https://www.listbox.com/member/?member_id=21175174&id_secret=21175174-792643f6> >>>> Your Subscription <http://www.listbox.com> >>>> >>> >>> >> >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
