raidz: fix space accounting after expansion

[Skountz]

Motivation and Context

After RAIDZ expansion, usable capacity is severely underreported because the deflation ratio (which converts raw
physical space to logical usable space) remains stuck at the original pre-expansion geometry. Both zfs list -o available and zpool list -o usable,available show values based on the old, less efficient parity layout.

Fixes #17784
Fixes #18199

Description

vdev_set_deflate_ratio() hard-codes txg 0 to compute the ratio. After expansion, new writes use a more efficient
geometry but the accounting never reflects this.

Fix in three parts:

1. Add vdev_deflate_ratio_current (computed with UINT64_MAX) alongside the existing vdev_deflate_ratio (txg0). Apply a capacity correction in spa_update_dspace() (for zfs list available space, quota enforcement, and space reservations) and in spa_prop_get_config() (for ZPOOL_PROP_USABLE / ZPOOL_PROP_AVAILABLE), using the difference between the two ratios on the free portion of each expanded vdev. Only the free portion is corrected because dd_used_bytes tracks old-geometry blocks at the legacy ratio; correcting allocated space would inflate available space beyond what can physically be written, which could cause pool suspension on a full pool. As old blocks are rewritten and vs_alloc shrinks, the correction automatically grows, converging to the full geometry correction after a complete rewrite.
   The original txg-0 ratio is preserved in vdev_deflated_space() for self-consistent metaslab accounting and persistent DN_USED_BYTES tracking.
   Note: zpool list default columns (SIZE, ALLOC, FREE) show raw physical space and are not deflation-adjusted; this is by design and unchanged.

2. Add vdev_get_deflate_ratio(vd, birth_txg) for per-block deflation using BP_GET_PHYSICAL_BIRTH(). Pass birth txg through dva_get_dsize_sync() and bp_get_dsize_sync(). Reuses existing reflow_node_t AVL tree infrastructure.

3. Add the raidz_expansion_accounting feature flag (depends on enabled_txg). Records the txg at which per-block deflation ratio tracking was enabled. Blocks born before that txg use the legacy fixed ratio (matching how they were originally accounted); blocks born at or after use the per-birth-txg ratio. Follows the standard OpenZFS feature lifecycle: the user enables it via zpool upgrade or zpool set, and it is activated on the next RAIDZ expansion via spa_feature_incr() in vdev_raidz_attach_sync(). If enabled on an already-expanded pool, it activates immediately in feature_enable_sync() (following the SPA_FEATURE_HEAD_ERRLOG precedent). Prevents born/free accounting mismatches for blocks written between a pre-patch expansion and the first expansion with this code.

Self-consistency is preserved: metaslab accounting uses the original ratio (alloc/free always balance), capacity
reporting is corrected at the spa level, and per-block accounting uses stable birth-txg ratios. No negative overflows
or positive leaks.

How Has This Been Tested?

• Full build on Ubuntu 24.04, make checkstyle clean.
• All existing raidz_expand tests (001-007) pass.
• Added five new ZTS tests:
  • raidz_expand_008_pos: Creates RAIDZ2-3 (1 data + 2 parity), writes 300MB, expands to 4 disks, verifies usable capacity increase (~100%) exceeds the raw 33% disk addition (confirming the deflation ratio correction is applied to ZPOOL_PROP_USABLE), verifies zfs dataset available also reflects the correction (exercises the spa_update_dspace() code path), checks alloc+free≈size consistency, verifies all accounting survives an export/import cycle, and verifies the USABLE property is not over-corrected (gap between USABLE and the geometric maximum must be proportional to fill level, catching any correction applied to allocated space instead of only free space).
  • raidz_expand_009_pos: Multiple sequential expansions (RAIDZ1 3→4→5→6 disks) with writes interspersed between each. Verifies that consumed space matches write size, free space decrease tracks consumed increase, and alloc+free≈size holds at every step.
  • raidz_expand_010_pos: Rewrite-between-expansions test (RAIDZ1 3→4→5). Deletes and rewrites all data after each expansion to force blocks from old geometry to new. Then fills the pool to capacity and verifies that total data written matches zfs available (deflated) within 30% tolerance.
  • raidz_expand_011_pos: Snapshot accounting across expansions. Takes snapshots before/between two expansions, deletes files from the active dataset, and verifies that snapshot referenced/used sizes remain stable and consistent. Also verifies pool-level accounting holds after snapshot destruction.
  • raidz_expand_012_pos: Feature flag lifecycle and born/free leak detection. Verifies raidz_expansion_accounting is enabled on new pools, activates on expansion, and survives export/import. Tests that deleting data written under both old and new geometries returns both pool-level allocated and per-dataset referenced to near-zero (no born/free accounting leak via bp_get_dsize_sync). Also tests second expansion, stress write/delete cycles, and the post-expansion enablement path (feature disabled at pool creation, expansion, then enablement triggers immediate activation).
• Tested on Debian 13 VM with ZFS kernel modules loaded.

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Note: spa_feature_t enum changed (new SPA_FEATURE_RAIDZ_EXPANSION_ACCOUNTING), which updates spa_feature_table size and the libzfs.abi/libzfs_core.abi files accordingly. dva_get_dsize_sync() signature changed and vdev_get_deflate_ratio() was added, but these are kernel-internal symbols (EXPORT_SYMBOL) only.

Checklist:

• My code follows the OpenZFS code style requirements.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[Skountz]

@amotin — following your feedback on #18170, I've implemented the per-block birth txg approach you described, using BP_GET_PHYSICAL_BIRTH() and the existing reflow_node_t AVL tree. Metaslab self-consistency is preserved.
Would appreciate your thoughts!

[owlshrimp]

Maybe it would be worthwhile to add some explicit tests of this functionality, to verify that the amount of data required to fill a pool is approximately equal to the remaining freespace that is reported?

For example, create a pool and then do a series of large writes interspersed with raidz expansions, while verifying that the size of the write, the amount of freespace remaining, and the amount of space consumed before/after are all within some tolerance of each other? It might also be worthwhile doing a version with where you do a ZFS rewrite each time inbetween expansions.

[Skountz]

@owlshrimp Good idea — added raidz_expand_009_pos (multi-expansion with writes between each step) and raidz_expand_010_pos (rewrites between expansions + fills pool to verify freespace accuracy).
All passing locally!

[owlshrimp]

...verifying that the size of the write, the amount of freespace remaining, and the amount of space consumed before/after are all within some tolerance of each other?

I suppose it would be more correct to say that the reported freespace before vs after should be approximately equal to the size of the write (with compression turned off, anyway). The various cases of "how much space should this have taken" with different stripe sizes makes my head hurt though, especially if you delete old data. Deleting old data is probably another area where tests verifying expected behavior would be useful.

[Skountz]

Mh, the two new tests cover both.
Test 009 does multi-expansion with writes at each step, checking that freespace decrease tracks allocated increase (not the logical write size directly — on RAIDZ those differ due to parity).

Test 010 does delete+rewrite cycles between expansions, then fills the pool to verify reported freespace actually matches writable capacity.

Let me know if you think that covers it or if you had something else in mind.

[owlshrimp]

Let me know if you think that covers it or if you had something else in mind.

Reading through the testing strategies, I think this probably covers everything I can think of offhand. Maybe there are weird edgecases with mixes of pre and post expansion data, but 09 seems like at least a very good sanity check.

What would we expect to happen to the total figures if some quantity of pre-expansion data is deleted? I presume the freespace would go up by a bit more than the amount of data deleted, due to the difference in parity ratio?

[Skountz]

What would we expect to happen to the total figures if some quantity of pre-expansion data is deleted? I presume the freespace would go up by a bit more than the amount of data deleted, due to the difference in parity ratio?

Right, pre-expansion blocks use more raw space per logical byte (worse parity ratio), so deleting them frees raw capacity that under the new geometry maps to more usable space. That's the convergence the commit message describes: as old blocks get deleted or rewritten, accounting gradually aligns with the new geometry.
Test 010 exercises exactly that path.

[owlshrimp]

I suppose every time you do an expansion (with these changes) the pre-expansion blocks belonging to files and snapshots look like they consume slightly more usable space than expected for their logical size.

Have you double checked things like the amount of space reported as consumed by old snapshots whose files have been deleted from the current view makes sense in some way? (eg. in zfs list -t all) Just thinking of potential edge cases.

[Skountz]

Yes, pre-expansion blocks genuinely cost more raw space per logical byte (worse parity ratio), so that's expected and correct.
The accounting stays self-consistent because bp_get_dsize_sync() always uses the block's physical birth txg to look up the deflation ratio, so a given block always produces the same dsize regardless of when you compute it.

Good call on snapshots though, I don't have explicit test coverage for that. I'll add one just after dinner (UTC+1 here!)

[Skountz]

Also added an export/import check to test 008 — vdev_deflate_ratio_current isn't persisted on disk, it's recomputed during vdev_open() on import. So if that recomputation ever regressed, all accounting would silently break after a reboot or pool reimport. Now verified it survives the cycle.

[owlshrimp]

I guess every time you delete a pre-expansion file that exists in a snapshot, the snapshot grows by the same slightly-more-than-logical amount of raw size as the file used to occupy?

[Skountz]

Yes exactly, the snapshot's used increases by the dsize of those blocks, which uses the old deflation ratio (via birth txg). So it reflects the actual raw cost of holding pre-expansion data. And test 11 already verifies this.

[owlshrimp]

Assuming one rewrote all the data (and didn't keep any snapshots) are there any lingering differences between an expanded array and one that was constructed natively, given this patchset?

[Skountz]

No, from this patchset's perspective there are no lingering differences. Once all data is rewritten under the new geometry, the per-block deflation ratios all reflect the current layout (identical to native), and the SPA-level capacity correction produces the same reported size as a natively constructed array.

The expansion feature itself does leave persistent metadata on disk (e.g., the raidz_expansion feature flag, expansion history), but that is unrelated to this patchset and does not affect space accounting or I/O behavior.

[Skountz]

This also follows up on the "time dependent deflate ratio" concept originally described by @ahrens in #12225 (comment)

[amotin]

This might be a bit controversial. IIRC DDT can add more copies to existing records, but there is only one ddp_phys_birth there. I guess the DDT stats may survive this, but it may need to be considered for other places.

[Skountz]

Added a comment. I checked the other callers of dva_get_dsize_sync() and it's only bp_get_dsize_sync()/bp_get_dsize() which use BP_GET_PHYSICAL_BIRTH, so the DDT stats path is the only place with this approximation.
Since ddt_phys_extend() preserves the original birth when adding copies, the approximation only affects DDT statistics, not persistent accounting.

[amotin]

"ddt_phys_extend() preserves the original birth when adding copies", but I suppose the new copies allocated with new width, while not sure with what deflate ratio. If old, then it is just a bit untrue, if new, then there will be a leak when deleted.

[Skountz]

The persistent accounting is safe here.
The physical free path (ddt_phys_free -> zio_free -> metaslab_free) uses raw DVA_GET_ASIZE(), no deflation ratio involved.

For dataset-level dsize, born and killed always operate on the same BP with the same phys_birth, so the deflation ratio matches on both sides.

The only inaccuracy is in ddt_stat_generate() (display-only DDT stats), which the comment at ddt_stats.c:59-66 already documents, if that's fine for you?

[amotin]

You are correcting reported pool stats, but not individual allocation classes above.

This also makes me wonder what is going to be corrected? Looking on this, I suppose the metaslab stats is not (hopefully not some mix), but looking on dsl_dataset_block_born() using bp_get_dsize_sync(), I suppose the dataset stats will be. And in case expansion happened before this patch, it will get some wrong mix of corrected an uncorrected values. I think this change might need a pool feature flag, tracking since what TXG we use corrected space usage.

[Skountz]

Fixed the per-class stats. The correction is now computed upfront and passed to spa_prop_add_metaslab_class(), so NORMAL_USABLE/NORMAL_AVAILABLE also get corrected.

On the feature flag: you raise a fair point. For blocks born post-expansion but before this patch, the old code used the txg-0 ratio at born time, while the new code would compute a different ratio at free time based on the birth txg.
That's a mismatch for those specific blocks. However for pre-expansion blocks there's no issue since the birth-txg lookup returns the original geometry anyway. The mismatch is bounded to blocks written between the expansion and the patch, and self-corrects as those blocks get rewritten. Do you think that's acceptable or would you prefer a feature flag to track the boundary?

[amotin]

Every time you allocate with one deflation ratio and free with another, you create a leak in absolute bytes instead of percents. While the last are compensated and only a cosmetic issue, the first I think will result either in unusable space or pool overflow. Neither of those are acceptable.

My main question actually was: can we properly correct metaslab stats instead of this sketchy correction?

[Skountz]

Right, I didn't think that through. The byte leak on that transition window is a real issue. When you say properly correct metaslab stats, do you mean updating vdev_deflate_ratio and the accumulated dspace at expansion completion, rather than the post-hoc correction?

[amotin]

I am not sure we can really fix already expanded vdevs, so my thinking is towards adding a new feature, using different (variable) deflate ratios since that TXG, and allowing rewrite to fix accounting of the old data with time.

[Skountz]

Thanks for the feedback! Added the raidz_expansion_accounting feature flag as you suggested.

It records the activation txg via enabled_txg and gates per-block ratio tracking in vdev_get_deflate_ratio() so blocks born before the feature use the legacy fixed ratio. No more born/free mismatch risk.

Also added raidz_expand_012_pos to test the feature lifecycle and born/free leak detection.

Looking forward to your thoughts, feels like the last piece of the puzzle to make it right!

[amotin]

spa_rdspace above calculated based only on normal class, while here you correct for all classes.

[Skountz]

Good catch, added a vd->vdev_mg->mg_class filter. Same in spa_prop_get_config().

[amotin]

This made me think what time should be used here. From one side dataset space accounting is done at BP_GET_BIRTH(bp) IIRC. From another, in case of dedup/cloning deflation might be from before the expansion, as pointed by physical time. In case of remap physical birth might be ahead of the logical, but we don't have raidz removal now...

[Skountz]

Added a comment. I went with physical birth because the deflation ratio should reflect the geometry when the DVAs were actually allocated. For dedup/clones the logical birth can be newer but the underlying DVAs still live in whatever geometry existed at physical birth, so I think it's correct.
As you say the remap case isn't a concern today, but would need to be revisited if raidz removal ever becomes a thing. Let me know if you see a case where this breaks down.

[amotin]

but the underlying DVAs still live in whatever geometry existed at physical birth, so I think it's correct

Right. We just need to make sure that datasets accounting use the same TXG, so that that numbers match.

but would need to be revisited if raidz removal ever becomes a thing

Thinking like that may result in RAIDZ removal never implemented as even more complicated. We should at least try to think ahead.

[Skountz]

Apologies, that came off wrong (my english is far from perfect!).
In the current code, both born and kill use BP_GET_PHYSICAL_BIRTH so the ratio matches for a given BP.
But if remap changes the physical birth to a post-expansion txg for a block originally born pre-expansion, that would create a born/free mismatch.

[Skountz]

@amotin You were right to push on this. After a few sleepless nights and ~20 VM resets from kernel crashes, I have a working RAIDZ top-level vdev removal branch (with tests!) that handles the accounting interaction properly. This PR is a blocker for it, I'll push once this is merged, let's keep the (hopefully) good surprise for later.

[owlshrimp]

How are you handling the reduction in redundancy (afaik) that has made raidZ removal impossible up until now? (being as removal works by mapping the vdev's blocks and their snapshots to virtual equivalents on other vdevs)

This is me guessing what the problem was from a barely-remembered 2nd hand description, but I think the issue was somewhere along the lines of "if you have a 1+5 wide raidZ removed and turned into a virtual device on a 1+2 wide raidZ, then loosing one disk in the new 1+2 radiz looses two blocks per stripe from the (now virtual) 1+5, killing it's ability to be read even though the underlying 1+2 still works."

[Skountz]

Good question again! What you describe is essentially RAIDZ shrinking, which amotin himself stated is "impossible even theoretically" in the original issue (#9013). The destination vdevs can't be RAIDZ at all, spa_vdev_remove_top_check() already rejects removal if any destination vdev has parity...so the remapped blocks land on mirrors or singletons.

The actual hard part was that the existing removal code copies raw byte segments, which doesn't work for RAIDZ because you need block boundaries to read through the parity layout. The solution builds a reverse map of block boundaries first, then copies each block individually through the RAIDZ I/O path.

(But that's another story, let's not make too much noise about it on this PR :) Feel free to DM me if you want more details!)

[owlshrimp]

this raises a different problem: you can only remove a raidz from an array where it is the one and ONLY raidz. unless, you only allow allocation on vdevs that are non-raidz out of a mix of {raidz, mirror, singleton} and even then you may make the fullness of the vdevs in the array very unbalanced. I think anyraid ran into a balance problem like this.

[owlshrimp]

I could see removal *to raidz vdevs* being possible only in the extremely undesirable situation where blocks in the virtual raidz were duplicated such the copies could collectively tolerate any combination of underlying raidz members being lost. (bringing a heavy space penalty for removal) there is iir some support for redundant data even on singletons, but afaik this would largely be entirely new machinery and may not play well with the way block pointers work

[Skountz]

Not quite, you can have multiple RAIDZ vdevs and remove one, as long as there are mirror/singleton destinations.

But let's save the rest for the removal PR.

[NJSilverbird]

How do I apply this patch? Or is there a big chance this will find its way into an official openzfs release soon?

[owlshrimp]

How do I apply this patch? Or is there a big chance this will find its way into an official openzfs release soon?

This patch is not ready yet and is still work in progress. If you attempt to apply it you are putting your data at risk.

After it's complete and passes review it will be merged into the main branch, and presumably will ship in a new release soon after that.

[Skountz]

@amotin, @behlendorf

Hey, just wanted to follow up. I rebased and repushed yesterday so it should be clean on top of current main.
All review comments have been addressed as well so far.
Let me know if there's anything I can do to help move it forward!

[savely-krasovsky]

I am not a ZFS developer, but it makes sense not to squash commits during the review phase at least, since it's hard to understand what changed in the last commit.

[Skountz]

I am not a ZFS developer, but it makes sense not to squash commits during the review phase at least, since it's hard to understand what changed in the last commit.

The contributing guidelines seems to disagree with you:

zfs/.github/CONTRIBUTING.md

Line 131 in 5dd9121

* Please attempt to limit pull requests to a single commit which resolves

But I agree that it can complicate things when the change spans multiple independent concerns. Here, the fix and its test are a single logical unit (and there was actually nothing to squash), so a single commit is both the rule and the right call. That said, it is still possible to review the changes file-by-file if looking at everything at once seems overwhelming.

[owlshrimp]

@amotin @behlendorf @ahrens This seems like a really useful series and a final missing piece for raidz expansion. Is there someone who might have the time to review this?

[behlendorf]

@Skountz would you mind rebasing this again and addressing the spa_feature_t enum reordering and related abi updates while you're at it. I'll try and make time for a full review this week or next.

[Skountz]

@Skountz would you mind rebasing this again and addressing the spa_feature_t enum reordering and related abi updates while you're at it. I'll try and make time for a full review this week or next.

The post-rebase checkABI passed, and everything is in order, I'm running the tests on a VM while the CI is doing its job just in case, but I guess we're good for the review once those are passing!

[owlshrimp]

It seems some qemu checks are failing?

[behlendorf]

Another rebase on master should sort out the CI failures.

[owlshrimp]

that issue with dedup looks......hairy. I hope it doesn't have bearing against this work, but I wouldn't bet against it.

[Skountz]

that issue with dedup looks......hairy. I hope it doesn't have bearing against this work, but I wouldn't bet against it.

@owlshrimp no bearing here, they're complementary. That mixed-epoch case (a dedup entry born before expansion, extended with a post-expansion DVA) is exactly what this PR's per-block accounting handles: each DVA's size comes from its own stored ASIZE and the block's physical birth, both immutable, so a block is freed with the same deflated size it was born with. No leak.

#18826 fixes a different path, where RAIDZ uses that single birth to pick the stripe width when reading the DVA. That's an I/O safety issue, not an accounting one.

Meanwhile I'm working through behlendorf's review here: making the feature READONLY_COMPAT instead of MOS. Testing now.

[owlshrimp]

that issue with dedup looks......hairy. I hope it doesn't have bearing against this work, but I wouldn't bet against it.

@owlshrimp no bearing here, they're complementary. That mixed-epoch case (a dedup entry born before expansion, extended with a post-expansion DVA) is exactly what this PR's per-block accounting handles: each DVA's size comes from its own stored ASIZE and the block's physical birth, both immutable, so a block is freed with the same deflated size it was born with. No leak.

#18826 fixes a different path, where RAIDZ uses that single birth to pick the stripe width when reading the DVA. That's an I/O safety issue, not an accounting one.

Does that mean this work might close that corner case, or would more work likely be required to fix that particular issue?

[Skountz]

that issue with dedup looks......hairy. I hope it doesn't have bearing against this work, but I wouldn't bet against it.

@owlshrimp no bearing here, they're complementary. That mixed-epoch case (a dedup entry born before expansion, extended with a post-expansion DVA) is exactly what this PR's per-block accounting handles: each DVA's size comes from its own stored ASIZE and the block's physical birth, both immutable, so a block is freed with the same deflated size it was born with. No leak.
#18826 fixes a different path, where RAIDZ uses that single birth to pick the stripe width when reading the DVA. That's an I/O safety issue, not an accounting one.

Does that mean this work might close that corner case, or would more work likely be required to fix that particular issue?

No, and it's not meant to. This PR is purely about space accounting; it guarantees a block is freed with the same deflated size it was born with, per DVA. It never touches the read path, so it does nothing to prevent the wrong-geometry read that #18826 addresses. That mixed-epoch block pointer still shouldn't exist, and #18826 is what stops it from being created (and contains pools that already carry one). The two are independent and both needed; neither closes the other.