femu/fdp: GC correctness + backpressure hardening (reworked)#194
Closed
huaicheng wants to merge 2 commits into
Closed
femu/fdp: GC correctness + backpressure hardening (reworked)#194huaicheng wants to merge 2 commits into
huaicheng wants to merge 2 commits into
Conversation
Two FDP GC victim-bookkeeping bugs surfaced while reviewing the reclaim-unit victim path, both independent of the issue #189 pos-aliasing crash and of the pqueue change-priority repair (PR #192). select_victim_ru() used the caller's reclaim group for the cross-RG cleanup. Under GC_NOISY_RUH_CUSTOM with nrg>1 the per-RUH victim queue can hold reclaim units from any reclaim group, yet the global-queue removal and the shared victim_ru_cnt-- referenced ssd->rg[rgid], the caller's group, rather than the victim's own. Remove the global twin from, and decrement the count on, ssd->rg[victim_ru->rgidx]; this equals rgid on every path that is not cross-RG, so single-group configurations are unaffected. ssd_trim_fdp_style() leaked cost-benefit victims. GC_GLOBAL_CB holds its full reclaim units in victim_ru_cb, but the trim/format drain emptied only victim_ru_pq, so those units were never freed and victim_ru_cnt was left stale. Drain victim_ru_cb as well. The two queues are mutually exclusive per RG mode and share the pos index, so the inactive one is empty here; guard the shared count against underflow.
With nrg>1 the FDP device wedged almost immediately: GC never triggered (GC_START=0) and writes failed with "No free RUs left in rg[0] / device full", for every GC strategy (reproduced with greedy and noisy). Root cause: a write with no placement directive is hard-coded to reclaim group 0 (ssd_stream_write sets rgid=0 when there is no NVME_DIRECTIVE_DATA_PLACEMENT). Each RG owns only tt_nru/nrg of the reclaim units, so rg[0] is exhausted after filling ~1/nrg of the device -- which happens during the initial sequential fill, before any overwrites have created GC victims. select_victim_ru() then returns NULL (empty victim queue), do_gc_fdp_style() returns -1 without logging GC_START, and the write path hits rg[0]'s empty free list and reports device-full. rg[1..nrg-1] are never used by default writes. Fix: add fdp_get_new_ru_spread(), which prefers the requested RG but falls back to any other RG that still has a free RU, and use it ONLY on the default host-write frontier (fdp_advance_ru_pointer RU rotation + the curr_ru recovery path), gated by !is_placement. NVMe FDP does not bind a non-placement write to a specific RG, so spreading default writes across RGs is spec-compliant; only the future write frontier changes RG, already-written data keeps its RG via its PPA. Directive (placement) writes stay pinned to their requested RG (is_placement -> fdp_get_new_ru, no spread). Cross-RG correctness (required once an active RU can live in a different RG than the request's rgid): - ssd_stream_write re-syncs rgid = ruh->curr_ru->rgidx and classifies a filled RU into its own RG's victim/full list (ssd->rg[ru->rgidx]); rus[] indexed by the new RU's rgidx. - do_gc_fdp_style frees the victim into victim_ru->rgidx (not the caller's rgid), or per-RG free_ru_cnt/free_ru_list drift and GC livelocks migrating fully-valid RUs without restoring free RUs to the RG under pressure. - fdp_advance_ru_pointer takes an allow_spread flag; the GC destination frontier passes false so GC stays pinned to the victim's RG (keeps ret_ru->rgidx == dest_ru->rgidx for the rus[dest_ru->rgidx] store). nrg=1 behavior is unchanged (the fallback loop is a no-op; rgid re-sync collapses to the sole RG). Independently Codex-audited (a first cut that only spread without the do_gc free-RG and GC-pinning fixes livelocked NOISY+nrg4; this version fixes all three). Validated on flex16: greedy+nrg4 and noisy+nrg4, which previously wedged at GC_START=0, now run cleanly under fio churn with crc32c verify, GC triggering and completing, no #189 assertion. This also makes the multi-RG NOISY cleanup path (victim_ru->rgidx) reachable and exercised at runtime.
Contributor
Author
|
Reworked and landed on master as |
huaicheng
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FDP garbage-collection hardening on top of the pqueue repair in #192, split
into focused commits. Reworked from the original version of this PR: the
nrg>1default-write-spread commit was dropped (it was incorrect, see below),and correctness/robustness commits were added and validated on real FEMU VMs.
Commits
fix cross-RG NOISY victim accounting and CB trim drain. Under
GC_NOISY_RUH_CUSTOMwithnrg>1,select_victim_ru()can return a victimfrom a different reclaim group than the caller's
rgid. The global-queueremoval, the delayed-GC put-back, the shared victim-count decrement,
mark_ru_free(), and the implicit-RU-change event all now key off thevictim's own reclaim group (
victim_ru->rgidx). For every non-NOISY paththis equals the caller
rgid, so the default greedy path is unchanged. Alsodrains the cost-benefit victim queue in
ssd_trim_fdp_style(), whichpreviously leaked reclaim units across a format/trim on a CB device.
run foreground GC until write pressure clears. The write path capped
foreground GC at a fixed number of passes; it now reclaims until
should_gc_high_fdp_style()reports the pressure has cleared, so a busydevice waits on reclamation instead of failing a write. Bounded by the
reclaim-unit population; exits immediately when no victim remains.
report device-full instead of asserting when GC stalls. When GC cannot
obtain a free destination RU,
do_gc_fdp_style()asserted (aborting theemulator). It now puts the victim back on its own reclaim group's queue and
returns -1, and
check_gc_ruh_available()no longer dereferences a NULLcurr_ru. The device degrades to a normal full outcome rather than crashing.point the initial active RU at the default reclaim group. FDP init left
ruh->curr_rureferencing the last reclaim group's RU, but a non-placementwrite uses reclaim group 0 and the write path advances
curr_ruthroughrg[rgid=0]'s bookkeeping. Withnrg>1this enqueued anrg[nrg-1]RU inrg[0]'s victim queue and later looked it up via its own group's queue witha stale heap position, a NULL dereference in
victim_ru_get_pri()thatcrashed the emulator during sustained overwrites (same class as [BUG] FEMU crashed on FDP mode in victim_ru_get_pri(a=0x0) #189, but for
nrg>1). Pointingcurr_ruatrus[0]fixes the default path.Dropped from the original PR
The
nrg>1default-write-spread commit is removed. Spreading default writes isitself spec-legal: a write with no Data Placement Directive "uses the Placement
Handle value 0h and the controller selects the Reclaim Group for that command"
(NVMe Base Spec 2.3, Flexible Data Placement). The problem was the
implementation: it broke FEMU's invariant that each
(RUH, RG)pair ownsexactly one active reclaim unit (the fallback allocated a new RU and
overwrote
ruh->rus[fallback], orphaning the existing one), and it did not pinvalid placement writes to their requested group. After commit 4,
nrg>1withdefault writes is crash-free but still uses only reclaim group 0. Spreading
default writes across groups, and pinning cross-group placement writes, need a
per-
(RUH,RG)active-RU model (the singlecurr_rupointer conflates them);that redesign is tracked separately.
Validation
Long-run GC stress on real FEMU VMs (Ubuntu guest, KVM), FDP mode, GC tracing on:
nrg=1,nru=256, ~42% fill, sustained randwriteGC_START == GC_DONE; 0 errorsnru=141,pgs_per_blk=1024, ~84% fill (the #189 config)GC_BACK_RESERTdeferrals + 1,405 GCs; 0 crashes/errorsnrg=2, default writesmaster); after commit 4: clean, GC on rg0 only, 0 errorsTotal tens of thousands of clean GC cycles with no assert, segfault, or
device-full false positive.