While I applaud the fact the FreeBSD foundation put money towards ZSTD integration, I don't approve this form of communication.
It isn't usual to present weekly changes to a PR to the mailing list, certainly not when they are relatively small. It's also rather odd that it primarily highlights your own changes. It would also be respectful to next time at least discuss it with the person currently maintaining the current PR, which is c0d3z3r0. It would also be more constructive to document any issues (such as zfs send compatibility) in the actual PR, instead of presenting something that is barely documented at all. Hence to me this seems an awful lot like a sugarcoated way of putting yourself into a spotlight. That being said: "ZSTD can require a lot of memory to compress at the higher levels" - This has been tested many times and has been proven to be false (or at least not relevantly big) many time by now. I suggest you read-up on the discussion about the memory allocation code in November-december. "if there is not enough memory, just store the block uncompressed, rather than waiting for memory" - If we start informing the mailinglist it would be great to do some with full information and not a TLDR (which leads to wrong conclusions from the reader). In this case it would be more usefull to explain there are multiple failover(retry) allocators if allocation fails and a last-restort-uncompressed option if all of those fail. "In previous benchmarks, this has also resulted in incorrect results." - This was indeed discussed in your own previous work about 2 years ago (which was quite a fierce debate) and frequent uncompressed storage was due to a design flaw in the allocation code of your original prototype. Considering the allocation code has been almost completely rewriten since then, I think it's muddying the water to point at those past benchmarks. The current version has been retested and such cases only exist in extremely unrealistic scenario's (we are talking "ZFS with ZSTD on 512MB ram" unrealistic) Don’t get me wrong: I'm very happy with the extra metrics. But I do not think it's needed to exaggerate it's usecase. Yours Sincerely, Kjeld Schouten-Lebbing -----Oorspronkelijk bericht----- Van: Allan Jude <allanj...@freebsd.org> Verzonden: maandag 22 juni 2020 23:20 Aan: status-upda...@freebsdfoundation.org; freebsd-fs <freebsd...@freebsd.org>; openzfs-developer <develo...@open-zfs.org> Onderwerp: [developer] ZSTD Project Weekly Status Update The FreeBSD Foundation has graciously sponsored me to complete the work I started a few years ago to integrate ZStandard compression into OpenZFS. After getting caught up on the changes that have occurred while I was away from the project, I started working on adding additional testing to catch possible errors. https://github.com/openzfs/zfs/pull/10278/commits/7ab55cbafe2aa126914e9d57550ad39c538967f9 - added kstat counters to track allocation failures. ZSTD can require a lot of memory to compress at the higher levels, and we do these allocations as non-blocking (if there is not enough memory, just store the block uncompressed, rather than waiting for memory). However, we want to count the number of occurrences of this condition, because a user will see a much lower than expected compression ratio if this occurs. In previous benchmarks, this has also resulted in incorrect results. My original design has been modified slightly to store additional information in a header before the compressed block contents. For LZ4 we used the first 32 bits to store the compressed size of the block (big endian encoded), so that we could avoid feeding the slack between that size and the end of the sector to the decompression function. For ZSTD we extended this to also store the zstd compression level, since we do not store this level of detail in the block pointer (the patch supports 40 levels, ranging from zstd1 - 19, and zstd-fast-1 - 1000). We have since further extended this to store the version of zstd that was used to do the compression. The idea is that this will allow us to safely upgrade to a newer version of zstd in the future, possibly be keeping both versions. There are some cases (nop-write, l2arc) where we might need to be able to recompression a block with the same settings (compression level, same version of zstd), to get the same checksum. https://github.com/openzfs/zfs/pull/10278/commits/d707accd14f289010118973fb5acee9a917d5d83 - To facilitate testing that the zfs zstd header (32bit size, 24 bit version, 8 bit level) is properly saved and restored from disk, I have extended zdb with a new -Z flag, to decode the information for a block. Over the next week I plan to work on adding tests around the inheritance of the zstd compression property. To avoid changing the user interface, and to keep the atomicity of setting the compression type (zstd) and level, the user does `zfs set compress=zstd-12 dataset`, but this is actually settings the property compress=zstd and compress_level=12. We want to ensure that this won't cause invalid configurations when the compress property is inherited. I also plan to dig into the issues around using zstd with compressed_arc disabled, and how that interacts with the L2ARC. The subject of the ability to disable the compressed_arc is on the agenda for the OpenZFS Leadership call tomorrow, so this will somewhat depend on the outcome of that discussion. There are also a number of compatibility issues around the 'zfs send' when using ZSTD compression. I would also like to thank the various members of the ZFS-on-Linux community to have helped with the integration and testing on Linux, a platform I am not very familiar with. -- Allan Jude ------------------------------------------ openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/Ta8f23ca0e635f6ae-M92768ced92cd0f10ddfd9107 Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
