zdb: account pending DDT-log frees in leak detection - #18820
Conversation
|
"and the physical free happens only when that log entry is flushed back into the DDT" -- it is not true. Physical free happens immediately when the record is only put into the log. Time when it is flushed into a DDT ZAP does not affect block allocation status. I haven't looked close (TLDR, I'd prefer more concise PRs) and not familiar with DDT handling in |
000d42b to
53ac0fe
Compare
zdb_count_block()'s --class filter resolves a block's allocation class by indexing the top vdev's metaslab array. An indirect vdev, left behind by a device removal, normally has no metaslabs; zdb synthesizes them in zdb_leak_init_prepare_indirect_vdevs() so that zio_claim() has somewhere to claim into. That only happens when leak tracking is enabled, so under -L the array is NULL and any block whose first DVA names a removed vdev dereferences it. Classify an indirect vdev from its allocation bias instead. The bias determines the vdev's primary metaslab group, and is available whether or not the synthetic metaslabs exist. For an indirect vdev with more than zfs_embedded_slog_min_ms metaslabs this also changes what the filter reports. The synthetic metaslabs carry no space maps, so they all look empty, and vdev_metaslab_init() moves the emptiest into the embedded log group; blocks in that range then classified as "other". Measured on a ztest pool with two removed vdevs, lowering the threshold so the move triggers: without the change 19 of the 1K blocks reachable through an indirect vdev counted under --class=other, and with it those same 19 count under --class=normal. Reporting the vdev's own class is both more useful and no longer dependent on whether -L was given. The metaslab lookup is kept for concrete vdevs, whose metaslabs can belong to an embedded log group that the bias alone does not describe. Add a test that removes the only disk data was written to, so every block is reached through the resulting indirect vdev, and runs the filter under -L. Signed-off-by: Kamil Monicz <kamil@monicz.dev>
With fast dedup, the last decref of a block lands in the DDT log, and the physical free happens only when the log entry is flushed back into the DDT. ddt_phys_decref() only decrements the in-memory refcount, the log append path issues no free, and ddt_phys_free() -- the only caller of zio_free() on this path -- runs from ddt_sync_flush_entry(). A pool examined inside that window contains blocks that no block pointer references but that are still allocated. zdb's leak detection never enumerates the log, so it misreports those blocks: as "leaked space" and a block traversal size mismatch in the generic accounting (print-only since d63f5d7), and as a fatal "obsolete indirect mapping count mismatch" when such a block sits behind a removed vdev's indirect mapping -- which is what intermittently fails the zloop CI job. Count the phys that the flush will free the same way as the deferred-free bplist, under a new "DDT pending free" bucket: reconstruct the BP, clear the dedup bit exactly as ddt_phys_free() will at flush, and feed it through zdb_count_block(). The selection mirrors ddt_sync_flush_entry(): an unborn phys owns no allocation, an obsolete ditto slot is freed whatever its refcount, and any other slot is freed once its last reference is gone. This fixes both symptom classes through one mechanism, since claiming also removes the extents from the indirect vdev's ms_allocatable that the obsolete-count recomputation scans. The accounting is exact, and does not require replaying the log against the DDT ZAP, because the import has already normalized them. ddt_log_load() replays each log in order so the newest record for a key wins, drops flushing-log entries duplicated in the active log, and skips everything at or before the flush checkpoint (whose frees were committed in the same tx). ddt_lookup() then searches the live tree, the active log and the flushing log before the stored objects, so a stale ZAP record is never consulted for a key the log describes. This code walks those already-normalized in-memory trees, so each key is represented exactly once. Non-ditto entries with a nonzero refcount are referenced by block pointers and remain covered by the traversal's lookup; pruned entries log a zeroed phys and are skipped by the birth check -- their blocks are still referenced and must not be claimed. A ditto slot is never the target of a logical BP, so claiming one cannot collide with the traversal. Genuine leaks still trip the existing checks; if a block pointer references a pending-free block (a corruption), the traversal's claim now fails visibly instead of passing silently. On a ztest pool captured inside the window, unpatched zdb reported 518 leaked extents and a 2080768-byte traversal mismatch; with this change the same pool reports no leaks, with 866 blocks accounted as DDT pending free. Add a ZTS test covering the generic window, the device-removal variant (which exits nonzero without this fix), that logged live entries and prune tombstones are not wrongly claimed, and that an entry resurrected by a new write before the log is flushed is not left pending. Signed-off-by: Kamil Monicz <kamil@monicz.dev>
53ac0fe to
3bf6ecc
Compare
The DDT code says so itself: Lines 2111 to 2114 in 1cec121 It's also observable: on a pool captured inside that window, unpatched On replaying the log against the ZAP — import already does it. |
Hmm. Seems I've missed that aspect. And I am not exactly happy about it, since as I suppose space on the pool won't get freed until the log is flushed, which may take a while after delete. @robn Could you comment here? |
3 participants
Motivation and Context
With fast dedup, the last decref of a block is recorded in the DDT log,
and the physical free happens only when that log entry is flushed back
into the DDT.
ddt_phys_decref()only decrements the in-memoryrefcount, the log append path issues no free, and
ddt_phys_free()--the only caller of
zio_free()on this path -- runs fromddt_sync_flush_entry():zfs/module/zfs/ddt.c
Lines 907 to 923 in 1cec121
Between the decref and the flush, the block is referenced by no block
pointer but is still allocated.
zdb's leak detection claims everyblock reachable from the pool root and expects the space maps to come
out empty; it never enumerates the DDT log, so it misreports blocks in
that window:
leaked space:lines(zdb.c#L6833)
and a
block traversal size != allocmismatch(zdb.c#L7841),
both non-fatal since d63f5d7; and
obsolete indirect mapping count mismatch(zdb.c#L7455)
when such a block sits behind a removed vdev's indirect mapping,
because the pending-free extent inflates the recomputed obsolete
count above the on-disk count.
The fatal variant is an intermittent
zloopCI failure (it surfaced onthe run for #18819, and master's own
zloopfails at a similar rate);ztestsnapshots pools with kill cycles, landing inside this window.Description
Two commits. The first is an independent
zdbcrash fix that the secondhappens to expose; it is separately backportable and has its own test.
1.
zdb: don't index an indirect vdev's metaslabs in the --class filterzdb_count_block()'s--classfilter resolves a block's allocationclass by indexing the top vdev's metaslab array. An indirect vdev, left
behind by a device removal, normally has no metaslabs;
zdbsynthesizesthem in
zdb_leak_init_prepare_indirect_vdevs()sozio_claim()hassomewhere to claim into, and that only happens when leak tracking is
enabled. Under
-Lthe array is NULL, so any block whose first DVAnames a removed vdev dereferences it. This needs no dedup at all -- any
pool with a removed vdev and
zdb -bL --class=...segfaults.Indirect vdevs are now classified from
vdev_alloc_bias, whichdetermines the vdev's primary metaslab group and is available whether or
not the synthetic metaslabs exist. The metaslab lookup is kept for
concrete vdevs, whose metaslabs can belong to an embedded log group that
the bias alone does not describe.
This is a deliberate behavior change, disclosed in the commit
message: for an indirect vdev with more than
zfs_embedded_slog_min_msmetaslabs, the synthetic metaslabs carry no space maps, so they all look
empty and
vdev_metaslab_init()moves the emptiest into the embeddedlog group -- blocks in that range previously classified as
other.Measured on a
ztestpool with two removed vdevs, with the thresholdlowered so the move triggers: 19 of the 1K blocks reachable through an
indirect vdev moved from
--class=otherto--class=normal. Reportingthe vdev's own class is more useful and no longer depends on whether
-Lwas given.2.
zdb: account pending DDT-log frees in leak detectionCount the phys that the next flush will free the same way the pool's
other frees-in-flight are already counted (the deferred-free bplist,
dp_free_bpobj, in-progress removal, deleted livelists), under a newDDT pending freeblock-type bucket.zdb_count_ddt_log_frees()walkseach logged table's active and flushing logs and mirrors
ddt_sync_flush_entry()'s free rule exactly -- an unborn phys ownsnothing, an obsolete ditto slot is freed whatever its refcount, and any
other slot is freed once its last reference is gone:
Clearing the dedup bit mirrors
ddt_phys_free()and keeps thereconstructed BP out of
zdb_count_block()'s dedup bookkeeping, so theexisting DDT refcount leak check is untouched. Claiming the block also
removes its extent from the indirect vdev's
ms_allocatable, which iswhat the obsolete-count recomputation scans -- so both symptom classes
are fixed by one mechanism, not just the prints.
DDTs without
DDT_FLAG_LOGhave no log to walk and are skipped, so thechange is a no-op for them. This is a
zdb-only change: no on-diskformat change, no kernel change.
The accounting does not require replaying the log against the DDT
ZAP, because the import has already normalized them.
ddt_log_load()replays each log in order so the newest record for a key wins, drops
flushing-log entries duplicated in the active log, and skips everything
at or before the flush checkpoint -- whose frees were committed in the
same tx
(ddt_log.c#L526-L528).
ddt_lookup()then searches the live tree, the active log and theflushing log before the stored objects, so a stale ZAP record is never
consulted for a key the log describes. This code walks those
already-normalized in-memory trees, so each key is represented exactly
once.
The remaining exclusions:
refcount > 0are still referenced and remaincovered by the traversal's log-aware
ddt_lookup(); they are notclaimed here.
zpool ddtprunelogs a zeroed phys for entries whose blocks are stillreferenced by live BPs; the nonzero-birth check skips those, so
pruned-entry tombstones are never claimed.
select slots 1-3 via
DDT_PHYS_FOR_COPIES(), verified lookup comparesagainst
ddp_trad[ndvas], andddt_phys_total_refcnt()excludes ditto-- so claiming one cannot collide with the traversal.
actually references a pending-free block (a real inconsistency), the
traversal's later claim now fails visibly instead of passing silently.
How Has This Been Tested?
Both fixes were reproduced and A/B'd against unpatched builds on real
pools (
ztest+zdb -e, userspace libzpool;zdbis a userspaceprogram either way, and libzpool compiles the same
ddt.c/ddt_log.c).Commit 1 -- pool with two removed vdevs,
zdb -e -p <dir> -bL --class=normal: unpatched exits 139 (SIGSEGV, core dumped); patchedexits 0. Classification is now independent of
-L: the completefiltered block-size histograms with and without
-Lare byte-identical(same SHA-256).
Commit 2 -- a fast-dedup pool captured inside the window (kill cycle
with
zfs_dedup_log_txg_maxpinned),zdb -e -p <dir> -bcc:leaked space16462848 != alloc 16498176 (leaked 35328)No leaks (block sum matches space maps exactly)Byte totals alone would not prove this --
zdb_count_block()adds ASIZEbefore filtering, so any BPs summing to 35328 would balance the books.
The identity check is by address: the 32 zero-refcount log entries
contain 45 DVAs, exactly equal as a set to the 45 extents unpatched
reports leaked (empty set difference). Two deliberately broken variants
confirm the test discriminates -- one that counts without claiming still
printed all 45 leak lines, and one that claimed the right sizes at
wrong addresses produced 32 claim errors and 46 leaked extents.
The ditto arm was exercised on a manufactured
LOG-without-FLATtraditional table (accepted by
ddt_configure(), which validates onlyunknown flag bits): patched counted all 882 pending frees with zero
leaked extents, while a variant with the ditto exception removed counted
10 and left 676 leaked extents / 7404544 bytes.
New ZTS coverage:
dedup_log_zdb_leak-- the generic window; a pre-delete check thatlogged live entries and
ddtprunetombstones are not claimed(guarding both exclusions); an entry resurrected by a new write before
the log flushes, which must leave nothing pending; and the
device-removal variant, which exits nonzero on unpatched
zdb.removal_indirect_class-- writes on the only disk, adds a second,removes the first, so every block is necessarily reached through the
resulting indirect vdev, then runs the
--classfilter under-L.make checkstyle(cstyle, shellcheck, commitcheck) andgit diff --checkclean; both scripts parse underksh -n. I have not run theZTS suite against a live kernel -- this environment has no module or
/dev/zfs-- so CI is the first privileged executor.Types of Changes
Checklist
Signed-off-by.