-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Zstd: Update bundled library to version 1.5.7 #18089
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
52ca857 to
03e9d2c
Compare
|
Memory is now unpoisoned when handing buffers out to Zstd and poisoned again when buffers are returned to the pool. Testing on the new Alpine runner looks good. I also restored SPDX license identifiers and fixed minor style issues in Compatibility with older ZFS releases and |
d35d2c3 to
492a996
Compare
|
I successfully build-tested In the latest push, tracing is disabled for kernel builds, as it caused issues on AArch64. This is now ready for review. |
|
I don't wanna be that guy, but... The traditional wisdom is that we can't upgrade the compression unless it outputs exactly the same compressed bytes for a given input as the previous version. This is because we calculate checksums over the compressed data, so to change the compression output would mean that the same data post-compression would not produce the same checksum. The more specific claim is something like: ZFS assumes that So the question here is does the newer zstd version guarantee the output is the same as our current version? The broader question, I suppose, is to what extent ZFS still requires that this is still the case. I don't think I know that off the top of my head (obviously I understand that it'll produce a different checksum, and will look different after encryption, but its not quite jumping out at me what the knock-on effects of that are). |
|
There's also sometimes performance changes between different versions and I know in the past newer versions have been slower for whatever reason. I don't personally care as much about that as correctness (well, within reason) but it still is worth being aware of. |
|
After posting this pull request, I realized there was prior discussion about compressor updates. This topic was discussed in the January 2022 Leadership Meeting. Some relevant quotes from that discussion are:
With that in mind, I’d like to understand what the recommended path forward is for upgrading Zstd. As far as I can tell, the vendored Zstd version is still at 1.4.5 from May 2020. |
|
@robn I've tackled those questions in #9416 , especially with encryption. That PR changed compression threshold logic, and was merged in 2.3.0. Since then I don't know about any problems after such change (but it essentially expects that if block was compressed before - it will compress after too, so we MAY have problems if compressor update make worse results). FWIW, we should not assume |
|
And, in "recompress on l2arc write" case looks like we already just skip such non-"recompressable idempotently" block https://github.com/gmelikov/zfs/blob/master/module/zfs/arc.c#L9273-L9280 |
Exactly, it's this assumption which can cause some minor problems in certain circumstances. The possible side effects really boil down to unexpected additional capacity usage and some optimizations being skipped which could impact performance. Let me try and summarize the state of things as of 2.4.0. @gmelikov @robn please chime in if I missed anything! 1. Uncompressed ARC + Compressed On-Disk + L2ARCAs @gmelikov mentioned this case has largely been addressed as of the 2.3.0 release. As long as the updated compressor code compresses blocks to the same size (or smaller) they will be written to the L2ARC. If re-compressed blocks are either 1) larger then before, or 2) encryption is enabled, and the blocks compress differently, then they aren't written to the L2ARC. Mind you this is still only a problem when the compressed ARC is disabled. It's enabled by default, so I wouldn't expect many users to encounter this. Even if they do it's a minor performance penalty until those files get rewritten with the new compressor. I think we've done what we can to address this one. 2. NOP WritesWhen a cryptographically secure checksum is used, such as SHA256, ZFS can detect if the new data being written is identical to the data on-disk for that block. If it is, the write can be skipped which avoids an unnecessary I/O and can save space in snapshots. If the data compresses differently with the new compressor the checksums won't match and this optimization will be skipped. My sense is NOP writes are uncommon enough that this really wouldn't be a problem in practice. 3. DeduplicationBlocks compressed with the new compressor may not deduplicate with blocks compressed using the old compressor. For users who are expecting a high-degree of deduplication this could be surprising after an update. We do now that we have richer options for managing dedup ( Given the above I don't think there are any major deal breakers. My main concern is the same as Rob's, we need to be able to verify (within reason) that no correctness issues were introduced with the updated version. One way to do that would be to support multiple versions of the compressor+decompressor and allow the version to be selected at run time. We already do something similar for the SIMD optimized checksum implementations to ensure they're all compatible. Since the zstd version is stored within the block header this would also potentially allow us to gracefully re-compress old blocks as needed. I'd be interested to know if others think it's worthwhile, it would definitely be a bit more work to maintain. |
|
With RAM getting more expensive and also CPU/RAM getting faster and faster, would it make sense to finally drop uncrompressed arc (and l2arc) in next zfs version? |
|
Looks like we have (a little bit patched) zstd v1.4.5 from May 22, 2020. I've skimmed through their changelogs https://github.com/facebook/zstd/releases , looks like there is a ton of performance enhancements (especially for small blocks <=128K, as we like) and some pretty critical fixes there. v1.5.7 was released in Feb 20, 2025, so it's pretty mature to use safely. I'd propose to just update our zstd as is without supporting multiple versions, BUT it would be best to do so only in major releases. It would be great to highlight notes about encrypted+uncompressed_arc+l2arc/NOP_write/dedup in our changelogs. I see these things as a necessity. |
|
Thanks everyone for the input! @behlendorf: I don't quite understand what you mean by "correctness" here. Do you refer to the idempotency property that @robn referred to and its potential effects in OpenZFS? To me, correctness with regards to compressor versioning would be Supporting multiple versioned Zstd implementations (or possibly other compressors) wouldn’t scale well across releases. Also, the January 2022 leadership meeting notes mention that
In any case, I think a strategy regarding future compressor updates is needed. My suggestion would be: right after a @gmelikov: To clarify my original pull request message: the intention was to fix Finally, I would like to drop the last commit with whitespace-only changes to |
|
It sounds like we have general agreement on how to handle updates to zstd going forward. In short:
This all sounds reasonable. @alex-moch my correctness concern above was was primarily around making sure updated versions of zstd are mature and stable before we pull them in. For me that means getting zstd updated early in our release cycle so we have time to develop confidence in the updated code. Well that and the interoperability testing as you've already done.
@alex-moch if if you can update the PR to make these changes we should be all set to get this merged. Could you also update the |
|
Late to the party here, but some comments. tl;dr I'm onboard with the upgrade plan/policy, great work! Lets go! I did feel like some past PRs attempting compression upgrades showed substantially worse performance. Unfortunately I can't find an obvious PR to point to; the discussion of compression in the issue tracker is... plentiful. I might be thinking of an LZ4 upgrade though; anecdotally it seems like people general have gotta broadly better performance out of zstd 1.5.x over 1.4.x. It would be nice, I suppose, to have something that could more easily compare algorithm performance before/after change, but we're not there (yet) and see no reason to pause right now. With a long lead time we should be able to shake it out. On running multiple versions, it would be way too difficult to do with the way things are currently put together, but I will note that I'm still working towards having a single mechanism for swapping algorithm implementations across ZFS (same idea as we have for eg AES, GCM, fletcher4, etc). I'm hoping to have more time for that kind of refactoring work this year, so hopefully the next time we tackle an upgrade we'll be able to just drop the new version into a different dir and compile them both in (or not, as we like).
We've been dancing around
I feel like there's possibly some opportunities here too. No actual ideas, just a sense that there could be things worth exploring. eg from one angle, physical rewrite is a way of understanding that two apparently-different blocks are actually the "same" in a different context. Not directly useful here perhaps, but now that the idea is introduced there may be more we can do with it. Anyway, none of that is directly related. I think that this PR is a good thing for us to do: we don't think there's any correctness issues, and its probably a good time in our history to be revisiting it and experiencing any difficulties first-hand (or not) so we can really be sure we know what we're dealing with. I'd love for (say) yearly math upgrades to be routine! First one is always scary :) |
This commit only replaces the bundled source and does not include any ZFS integration changes. Because the build depends on integration adjustments, it will fail until the accompanying integration commit is applied. Upstream release: https://github.com/facebook/zstd/releases/tag/v1.5.7 Signed-off-by: Alexander Moch <[email protected]>
This commit builds on the previous zstd library update and adds the necessary ZFS integration and build system changes required to make zstd 1.5.7 compile and function correctly. Changes: - Add zstd_preSplit.c (new in 1.5.7) to all build systems. - Enable x86_64 assembly in userspace (huf_decompress_amd64.S). - Disable assembly in kernel for RETHUNK/IBT compatibility. - Disable intrinsics in kernel for EL10 x86_64-v3 baseline. - Disable tracing in kernel builds for AArch64 compatibility. - Fix ZSTD_isError symbol renaming with __asm__ directive. - Rename abs64 to ZSTD_abs64 (FreeBSD kernel conflict). - Fix bitstream.h attributes (MEM_STATIC -> FORCE_INLINE_TEMPLATE). - Remove xxhash.c from BSD build (now header-only). - Update symbol names in zstd_compat_wrapper.h. - Ignore checkstyle for zstd-in.c. Kernel assembly disabled for security mitigation compatibility. User space retains full performance. Signed-off-by: Alexander Moch <[email protected]>
The Zstd context mempool can reuse buffers that were previously poisoned under AddressSanitizer, leading to false-positive use-after-poison reports during zloop and other stress tests. Explicitly unpoison memory when handing buffers out to Zstd and poison the user-visible region again when buffers are returned to the pool. This makes the allocator ASan-correct while preserving existing pooling behavior. Also fix non-standard void * pointer arithmetic in zstd_free() and remove an early return in zstd_dctx_alloc() so kmem_type/kmem_size are always set on pool hits. This only affects ASan bookkeeping in user space, does not change runtime behavior in non-ASan configurations, and does not affect on-disk formats. Signed-off-by: Alexander Moch <[email protected]>
When updating Zstandard to version 1.5.7 the SPDX license identifiers were lost. This commit restores them. Signed-off-by: Alexander Moch <[email protected]>
|
I just pushed the update:
|
|
One more thing that just occurred to me and might be worth explicitly adding to the update policy:
This makes the import easy to verify against upstream sources during review and later by anyone auditing OpenZFS’ git history. It also reduces supply chain risk by ensuring we have clearly auditable provenance for third-party code. |
Good thought, let's add it. Then this PR should be ready to go. |
Add the Zstd update policy to the subtree README. Also update the documented location of zstd-in.c to match upstream changes, and normalize naming from 'ZSTD' to 'Zstd'. Signed-off-by: Alexander Moch <[email protected]>
|
Just added it. I went with stronger wording: must instead of should. |
@behlendorf Since @robn opened this topic too in this answer above, whats your opinion? Is 2026 the time to bring back this topic and finally drop uncompressed arc? |
This pull request updates the bundled Zstandard (zstd) library to version 1.5.7 and adds the necessary OpenZFS integration and build system changes required to make zstd 1.5.7 compile and function correctly.
Motivation and context
This pull request is motivated by build failures observed when building with GCC using
-Og, as reported in:The version of Zstandard currently shipped with OpenZFS ignores
-DZSTD_NO_INLINEdue to how inlining is enforced aroundxxhash.cin the upstream sources.Fixing this issue for the currently bundled Zstandard version would require carrying local patches to upstream code.
Zstandard 1.5.7, among other changes, removes the standalone
xxhash.csource and adjusts inlining behavior such that builds with-Og -g -DZSTD_NO_INLINEsucceed. As a first step, this pull request updates the bundled Zstandard version in OpenZFS to 1.5.7 and performs the required integration changes. This is considered the cleanest approach for themasterbranch.Updating the Zstandard version in released OpenZFS stable branches is likely not an option. The approach taken here is therefore to fix
masterfirst and then evaluate whether a targeted workaround is feasible for OpenZFS 2.4 and possibly earlier releases.Description
The latest upstream Zstandard 1.5.7 release has been imported into
module/zstd. Where required, corresponding changes to the OpenZFS build system and integration code have been applied.This pull request intentionally deviates from the “one commit per pull request” model:
This separation is intended to simplify review and improve traceability of upstream versus OpenZFS-specific changes.
A third commit will add SPDX license identifiers to newly imported files to match existing OpenZFS conventions.
The upstream Zstandard sources are imported without reformatting or style adjustments, which intentionally deviates from OpenZFS coding style guidelines to keep the vendor update easy to track and review.
How has this been tested?
--enable-linux-builtinTodos before marking this ready for review
Add SPDX license identifiers to newly imported files.Remove temporary allocator bypass and implement proper fix.Test against the new Alpine Linux CI runner.Perform additional compatibility checks with older ZFS releases.Check kernel builds with--enable-linux-builtin.Types of changes
Checklist
Signed-off-by.