FDT: don't extend dedup across RAIDZ expansion - #18826
Conversation
|
@Zaczero I haven't looked inside, but CI looks really unhappy. |
9fdff66 to
4edf963
Compare
When a fast-dedup (FDT) entry is written with more copies than it currently holds, zio_ddt_write() issues a child write to allocate the shortfall DVAs and, at READY, extends the entry via ddt_phys_extend(). A later write that wants still more copies adopts the in-flight lead as its own child, forming a chain of leads that complete oldest-first. Three latent defects in that path make it unsafe to add a second reason to reject an extension at READY (see the following commits): 1. The chain shares a single rolling rollback point, dde_rollback_phys. On a successful child it was advanced with ddt_phys_copy() from the live dde_phys, which by then may already include a newer, not-yet-committed lead's DVAs; and on failure it was cleared unconditionally even while a later lead still needed it. A failed lead could then retain an uncommitted DVA or drop a committed one. Advance the rollback point with the completing child's own private BP (ddt_phys_extend(orig, v, zio->io_bp)); clear it only when this was the last lead (dde_lead_zio[p] == NULL). 2. zio_ddt_child_write_ready() unwound the per-metaslab-group allocation queue depth unconditionally on the gang-reject path, but that depth is only incremented for throttled async allocations. A synchronous or unthrottled child underflowed the counter. Gate the decrement on ZIO_FLAG_ALLOC_THROTTLED, which is set iff the increment happened. 3. An intentional EAGAIN (the dedup-to-plain-write fallback) reached spa_log_error()/zfs_ereport_post() in zio_done() before it was classified as a retry, logging a persistent on-disk data error and an ereport for a hole BP. Consume it after transforms, before error reporting. A deliberate extension-refusal EAGAIN has two producers -- zio_write_gang_block()'s allocation-stage refusal to gang a DDT extension child (a mixed gang/non-gang BP would be illegal) and the READY-stage refusal for an already-ganged live phys (joined by the RAIDZ geometry check in a following commit). Disable dedup, clear io_error, and reexecute as a plain write; the retry terminates because zp_dedup transitions monotonically to false. A backend EAGAIN can also take this path and gets one non-dedup retry before ordinary error handling. These are pre-existing on the gang-extension path. No functional change for the common single-lead extension. Signed-off-by: Kamil Monicz <kamil@monicz.dev>
4edf963 to
2b27d15
Compare
A RAIDZ vdev selects each block's logical stripe width from the block pointer's single physical birth (vdev_raidz_get_logical_width()). After a raidz expansion, an older block reads at its birth-time width and its recorded ASIZE always covers that (wider, more-parity) footprint, so implied == owned. If a block pointer instead names a DVA that was allocated at the new (narrower) width but is stamped with an older physical birth -- which fast-dedup extension across an expansion boundary can produce, and which older FDT code could also write -- then the birth-selected map covers more space than the DVA actually owns. A self-heal or resilver of that copy would map that oversized footprint and write past the DVA's allocation, into an adjacent block -- in any build: the nearby DEBUG-only VERIFY (asize_new <= asize) only checks that width geometry is monotonic in psize terms; nothing compares the implied footprint against the DVA's owned allocation. Detect the condition (vdev_raidz_io_exceeds_dva(): the birth-width footprint exceeds the matched DVA's owned ASIZE, gang headers compared against their constant header extent) and reject it with EIO in vdev_raidz_io_start() -- for reads as well as writes -- before taking the reflow rangelock or constructing a map. A zfs_dbgmsg breadcrumb identifies the rejected RAIDZ vdev and geometry. vdev_raidz_io_done() returns for the resulting NULL map, so no column reads, no vdev_raidz_read_all() re-issue, and no repair writes from a replacing/spare mirror below the RAIDZ are ever emitted. The bad copy fails with a checksum/IO error (degraded redundancy) instead of corrupting a neighbor; the other copies are unaffected and read normally. The classifier only fires for the exact malformed DVA: a legitimate block's implied footprint never exceeds what its DVA owns (a gang child may own more than the final BP implies), so it is never rejected. When the historical and physical widths differ, a direct top-level dispatch whose BP matches no DVA of the addressed vdev is impossible; treat it as the same malformed condition and reject (fail closed) rather than assuming safety. The rejection diagnostic reports the required (birth-width) ASIZE against the DVA's owned ASIZE. Signed-off-by: Kamil Monicz <kamil@monicz.dev>
2b27d15 to
bc74d28
Compare
A block pointer carries one physical birth for all of its DVAs, and RAIDZ uses that birth to choose the stripe width to read every DVA at. Ordinary allocation upholds the implied invariant -- every DVA in a BP shares one geometry epoch -- because all of a BP's DVAs are allocated in one txg. Fast-dedup extension breaks it. ddt_phys_extend() appends newly allocated DVAs to an existing flat phys while keeping the entry's original ddp_phys_birth, and every referencing BP is filled with that old birth. If the entry was born before a raidz expansion and the added DVA is allocated after it, the two DVAs live in different width epochs but share one birth. The BP cannot represent the newer DVA's geometry epoch, so RAIDZ reads that copy at the wrong layout (a checksum error in the common case), and -- absent the containment added separately -- a self-heal maps an oversized footprint that overruns the DVA's allocation. Reject the extension at READY when a newly allocated DVA would land on a raidz top vdev whose logical width at the entry's birth differs from its width at the allocation txg (vdev_raidz_same_logical_width()). The write falls back to an ordinary non-dedup write -- a fresh, coherent BP whose DVAs share one epoch -- via the existing EAGAIN reexecute path, exactly as the gang-mismatch reject already does. Same-epoch extensions, fresh entries (birth 0), traditional tables, and non-raidz tops are unaffected. No on-disk format change. The intentional EAGAIN is consumed in zio_done() into ZIO_POST_REEXECUTE (dedup disabled, io_error cleared) rather than propagated as an error. Evaluating both reject reasons under dde_io_lock also corrects an existing quirk: a DDT child that fails allocation (e.g. ENOSPC) on a ganged entry now propagates its real error instead of being overwritten with EAGAIN, matching the non-gang path. Add raidz_expand_008_pos: write a copies=1 dedup block, expand 3->4, then request copies=2 of the same content; assert the entry is not extended across the width boundary (it stays a single old-width DVA and the second file gets a fresh non-dedup BP), while a same-epoch control still extends normally. The test pins each DDT dump to the pool GUID and drains the FDT log (a bounded sync loop until both log headers are empty) before asserting persistent DDT entry counts. Signed-off-by: Kamil Monicz <kamil@monicz.dev>
bc74d28 to
842f273
Compare
|
@Zaczero I don't care what AI helps you, but next time you submit PR with 6 pages of description I will not read it. I have other things to do. You'll have to look for other reviewers. And good luck with that! |
I specifically tried to be verbose and clear and structure it in ways that make finding key information fast. It's all sectioned off the standard template you use here. https://github.com/openzfs/zfs/blob/master/.github%2FPULL_REQUEST_TEMPLATE.md That one. I was on the assumption that on a project like zfs, and with such template structure, more explanation and reasoning is better. |
The template is there to make people to write at least something. If you look around, nobody else aside of several new AI-powered individuals here write so much, simply because writing is supposed to be harder then reading, and only AI here is literally payed by the word. Are you saying you've written 6 pages of PR description plus 3 additional unique pages of commit messages by hands? ... Whatever. Please respect the reviewers. |
I don't write much by hand these days; I find that reading and verification are much more efficient. I am also starting to use more voice typing :-). I decided to be clear and verbose with my reasoning because ZFS is a production filesystem that is used by many. At least to me, a "freshie," this appears much more approachable, but I can see that it could be too verbose for vets. I think for reviewers, the most important section is just Motivation and Context, as the rest is mostly redundant and is there to provide extra, "just in case" info. Motivation and Context focuses on the full story: cause, resolution, workarounds, and impact—everything that seems like it should matter for PRs into ZFS (in my opinion). I additionally like to use GitHub-rendered code blocks because they enrich the story without needing to jump around the pages. Unfortunately they do increase the page size too. |
|
I have now added a collapsible section, so the default view should contain pretty much all that is necessary for a review, leaving optional things optional. In my taste, but it is slightly deviating from the PR template now. I was thinking this, that the template could be improved, at least by removing the Description section, which feels redundant in favor of Motivation and Context. I cannot think of a reason why there are two of these sections to be filled in, and this would save everyone time. |
| if (zio->io_flags & ZIO_FLAG_ALLOC_THROTTLED) { | ||
| for (int i = 0; i < BP_GET_NDVAS(zio->io_bp); i++) { | ||
| dva_t *d = &zio->io_bp->blk_dva[i]; | ||
| metaslab_group_alloc_decrement(zio->io_spa, | ||
| DVA_GET_VDEV(d), zio->io_allocator, | ||
| METASLAB_ASYNC_ALLOC, zio->io_size, zio); | ||
| } |
There was a problem hiding this comment.
The change itself seems to make sense, but makes me wonder what this code is doing here in DDT at all, and why it is not in zio_ready(), near the metaslab_class_throttle_unreserve()?
It makes me thing that only this specific commit would worth a 3 separate PRs.
There was a problem hiding this comment.
Aside of vdev_raidz_psize_to_asize() refactoring, which is nice but irrelevant, this second commit looks excessively verbose for what it really is -- a partial workaround for fixed extremely rare bug, that has to stay in the code forever. I don't very like the traversal over the DVAs list. And vdev_raidz_dva_extent_t definition just make me think about your 6 pages of PR description -- you should restrain yourself. ;)
| if (extent == VDEV_RAIDZ_DVA_EXTENT_UNMATCHED) { | ||
| zfs_dbgmsg("%s: rejecting direct raidz " | ||
| "vdev %llu at offset %llx size %llu: " | ||
| "no BP DVA owns it", |
There was a problem hiding this comment.
I'd say this worth assertion, not a dbgmsg. If it should not happen, then it should be asserted.
| if (vd->vdev_ops == &vdev_raidz_ops && | ||
| !vdev_raidz_same_logical_width(vd, phys_birth, | ||
| zio->io_txg)) |
There was a problem hiding this comment.
If anything, I'd say this should be a vdev type method.
But by this time we already passed allocation throttling and allocated a space, just to find the issue, free it and restart again. While this should work, I'm thinking if it would be a less of a code pollution just to block the BP expansion if the RAIDZ expansion feature is active on the pool. I really don't know anybody using copies=2 on their pools, especially in combination with dedup and RAIDZ expansion.
3 participants
Motivation and Context
Summary. Fast dedup (FDT) and RAIDZ expansion both shipped in 2.3.0. Used
together they can produce a block pointer that silently loses the redundancy the
user paid for and — during a scrub or resilver — overwrites an adjacent block
on disk. The root cause: a dedup entry born before an expansion can later be
extended with a DVA allocated after it, placing two RAIDZ stripe-width epochs
under the entry's single physical birth. RAIDZ uses that one birth to pick the
width for every DVA of the block, so it reads the newer DVA at the wrong
(older, wider) geometry. The fix never creates such a block (producer guard,
commit 3), fails cleanly on pools that already carry one (containment, commit 2),
and first hardens the retry path both rely on (commit 1). No on-disk format
change.
What has to happen for the bug. All three must hold:
feature@fast_dedup, 2.3.0+) anddedupisenabled on the dataset.
zpool attach) after some dedup data hadalready been written to it.
copiesvalue, so FDTextends the existing pre-expansion entry with a freshly allocated DVA on the
now-wider RAIDZ vdev.
The result is one dedup entry with a DVA at the old width and a DVA at the new
width, both under a single pre-expansion birth.
Why it happens. RAIDZ makes geometry a function of time. After an expansion,
a block is read at the stripe width that was in effect when it was born,
selected from the BP's single physical birth:
zfs/module/zfs/vdev_raidz.c
Lines 2712 to 2713 in 9b7642d
That is correct only because, historically, every DVA in a BP was allocated in
one txg — one geometry epoch per BP. FDT extension breaks that assumption: when
an existing dedup entry gains copies,
ddt_phys_extend()appends the newlyallocated DVAs but keeps the entry's original birth:
zfs/module/zfs/ddt.c
Lines 800 to 810 in 9b7642d
and every referencing BP is filled with that old birth:
zfs/module/zfs/ddt.c
Lines 708 to 729 in 9b7642d
So the entry born before the expansion, extended with a post-expansion DVA,
yields a BP carrying two width epochs under one birth — and RAIDZ reads the newe
DVA at the older width.
What it causes. Two tiers:
newer DVA's width epoch, so that copy generally fails checksum verification
(small or layout-invariant blocks can escape the misparse, but the redundancy
contract is broken either way). While the original copy is healthy this is
masked — so the redundancy the user paid for is silently absent — until the
original copy is lost, at which point the block is unreadable even though the
newer copy's data is physically intact.
≥ the new-width allocation (the nearby DEBUG-only
VERIFY3U(asize_new, <=, asize)only checks that geometric direction — it never compares a map's footprint
with the DVA's owned allocation, so no build detects the overrun). A scrub or
resilver repairing the extended copy maps that oversized footprint and
writes past the DVA's allocation, into an adjacent block. For a 128 KiB
block on a RAIDZ1 expanded 3→4 at
ashift=12, the map covers 48 sectors overa 44-sector allocation — a 16 KiB overrun.
A single per-DVA birth cannot be represented without an on-disk format change, s
the correct fix is to never create the mixed-epoch BP, and to contain repairs on
pools that already have one.
Detecting an affected pool.
zdb -DDDDD <pool>shows a flat DDT entry withone birth but two DVAs of different
ASIZE(e.g.
DVA[0]=<...:30000> DVA[1]=<...:2c000>).Workaround until patched. Avoid raising
copies(or otherwise re-writingidentical content at a higher copies value) on RAIDZ pools that were expanded
after the data was first written;
dedup=offavoids the producer entirely.Provenance. Affected releases are 2.3.0 onward. The premise was noticed
during review of #18324 (that FDT can add copies while retaining one birth,
here and
here) and
correctly resolved for space accounting (free walks per-DVA ASIZE). This PR
completes that analysis for the I/O path, where the single per-BP birth does
select geometry and therefore mis-parses the new-width DVA.
Additional Details (optional read)
Description
Three commits, independently backportable:
1.
zio: make chained DDT extension retry transactional— hardens theexisting extension-retry path, which must be correct before a second reason to
reject an extension is added. Fixes three latent defects that also affect the
shipped gang-mismatch reject: a shared rollback snapshot that could revert an
entry to a state keeping an uncommitted DVA or dropping a committed one (now
advanced only by each completed child's own BP, and cleared only by the last
lead); an unconditional alloc-throttle-queue decrement that underflowed for
synchronous children (now gated on
ZIO_FLAG_ALLOC_THROTTLED); and anintentional
EAGAINthat reachedspa_log_error()/zfs_ereport_post()before being classified as a retry, logging a persistent error for a hole BP
(now consumed before error reporting). The retry classification keys on the
EAGAINitself — an allocating logical dedup write — so it covers bothdeliberate refusal sites (the allocation-stage gang refusal in
zio_write_gang_block()and the READY-stage rejects) with one consumer, andit terminates because the retry monotonically disables dedup.
2.
vdev_raidz: contain repair maps that exceed their DVA allocation—protects pools already carrying a malformed entry. When a birth-selected map's
footprint exceeds the matched DVA's owned
ASIZE(vdev_raidz_map_exceeds_dva(),gang headers compared against their width-invariant header extent), the whole
map is rejected with
EIOinvdev_raidz_io_start()— for reads as well aswrites — before any column I/O.
vdev_raidz_io_done()short-circuits arejected map, so no column reads, no
vdev_raidz_read_all()re-issue, and no repair writes from a replacing/spare mirror below the RAIDZ
(which issues its own BP-less repairs)
are ever emitted. The bad copy fails cleanly instead of overrunning a
neighbor; other copies read normally. A legitimate block's implied footprint
never exceeds what its DVA owns (a gang child may own more than the final BP
implies), so it is never rejected. When the historical and physical widths
differ, a direct top-level dispatch whose BP matches no DVA of the addressed
vdev is likewise treated as malformed and rejected (fail closed), and the
rejection diagnostic reports the required birth-width
ASIZEagainst theDVA's owned
ASIZE.3.
zio: prevent DDT extension across a RAIDZ expansion boundary— theproducer fix. At extension READY, if a newly allocated DVA would land on a
RAIDZ top vdev whose logical width differs between the entry's birth and the
allocation txg (
vdev_raidz_same_logical_width()), reject the extension. Thewrite falls back to an ordinary non-dedup write — a fresh, coherent BP — via
the existing
EAGAINreexecute path, exactly as the gang-mismatch rejectalready does:
zfs/module/zfs/zio.c
Lines 3753 to 3762 in 9b7642d
Same-epoch extensions, fresh entries (birth 0), traditional tables, and
non-RAIDZ tops are unaffected. No on-disk format change.
How Has This Been Tested?
Coverage is called out per commit, since one part (the containment) cannot be
exercised by a committed test — see below.
raidz_expand_008_pos. Writes acopies=1dedup block, expands RAIDZ1 3→4, crosses the completion marker, thenrequests
copies=2of the same content. On unpatched code the entry isextended across the width boundary (a flat entry with two different-
ASIZEDVAs); with the fix it is not (the entry stays a single old-width DVA and the
second file gets a fresh non-dedup BP), and a same-epoch control still extends
normally. The test pins each DDT dump to the pool GUID and drains the
FDT log (a bounded sync loop until both log headers are empty) before counting
persistent DDT entries — the previous revision's form could inspect the wrong
pool (
cachefile=nonewith a barezdb) and counted log-resident entries.The test was also driven red→green in a userspace
libzpoolharness beforewriting the ZTS form.
ZTS test: its closing
No known data errorsassertion follows a guard-triggeredEAGAINretry, so a spurious persistent error would fail it. The chained-leadrollback is verified by analysis (every interleaving) and exercised
stochastically by
zloop; like other multi-in-flight-lead fixes it has nodeterministic committed test.
libzpoolharness, nocommitted regression test. A deliberately crafted malformed entry (a
new-width DVA spliced onto an old-birth entry) is read and rewritten through
the real RAIDZ path. With the fix, both the read and the write return
EIOwith 0 leaf columns issued (measured via
vdev_statop counters), and acanary spanning every leaf byte the oversized old-width map reaches beyond the
DVA is unchanged; with the containment removed, the read instead returns
ECKSUMand issues 8 leaf columns, so the harness has teeth. I am flaggingplainly that this commit has no committed regression test: once commit 3
lands, the malformed on-disk state can no longer be produced through supported
operations, and the only committed routes are a debug-only guard-bypass tunable
(declined — production-code cost) or a pre-built malformed-pool image. I can add
a malformed-pool-image ZTS as a follow-up if maintainers prefer; the harness
patch and red/green logs are available.
-Werror, debug);make cstyle,make commitcheck, andgit diff --checkclean; each commit builds standalone. I have not run thefull ZFS Test Suite against a live kernel on this host; CI runs it.
Types of Changes
Checklist
Signed-off-by.