In D26196#582083, @mmacy wrote:

In D26196#582060, @mahrens wrote:

Could we make this change upstream (in OpenZFS) rather than having to maintain the diff in freebsd? Or, would someone mind explaining when we would change the contrib files in FreeBSD vs changing them upstream?

Changes to contrib should always be made in the following order when possible:
a) update upstream
b) update vendor branch with latest changeset
c) merge vendor branch in to HEAD

Thus the first step is to open a PR with openzfs.

We may eventually relax the ordering if this proves too onerous for something as tightly coupled as ZFS and transition to "eventual consistency" whereby contrib + vendor + upstream are eventually the same.

Could we make this change upstream (in OpenZFS) rather than having to maintain the diff in freebsd? Or, would someone mind explaining when we would change the contrib files in FreeBSD vs changing them upstream?

Could we change the contrib files upstream (OpenZFS), rather than maintaining the diffs?

the interfaces look good to me.

Thanks!

So you only have one new compression value in the BP? Have you looked at how this interacts with L2ARC when compressed ARC is disabled? See arc_cksum_is_equal(), I don't think this will work right, because you won't be able to reproduce the on-disk compressed data without knowing the compression level.

@seanc OpenZFS/illumos has been at 4K chunk size since ABD was introduced. I'm not aware of any discussion around changing that.

FYI - we use 1K ABD chunks at Delphix (on illumos), and we've found that up to 40% of memory can end up being wasted because of memory fragmentation. At least on illumos, the 1K ABD kmem cache will use slab size 4K (one page), so there are 4 chunks per slab. After eviction, we may have many partially-full slabs, which wastes the free chunks. We are considering implementing a kmem_move callback which would allow the free chunks to be consolidated, reducing the potential for memory waste. But it isn't done yet.

looks good. Happy to have this upstream too.

You can use the instructions here: https://github.com/openzfs/openzfs to submit a pull request and get regression tests. Then solicit code reviews to developer@lists.illumos.org.

What is the use case for this? AFAIK it isn't possible to create all those holds with one zfs command (there's just "zfs holds -r <snapshot>" which creates a hold on one snapshot per filesystem.

This is a good start, but you also need to update the manpage and help message (see get_usage()).

Looks fine to me. FWIW, illumos appears to have flsll() in userland but not kernel. It's implemented similarly to highbit().

looks good to me.