From 11191fb04e1385d2530456a566ff9bf592c28311 Mon Sep 17 00:00:00 2001 From: Huaicheng Li Date: Tue, 21 Jul 2026 02:19:55 -0400 Subject: [PATCH 1/2] femu/fdp: fix cross-RG NOISY victim accounting and CB trim drain 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. --- hw/femu/bbssd/ftl.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/hw/femu/bbssd/ftl.c b/hw/femu/bbssd/ftl.c index c9a3328fd8..43bd9f5291 100644 --- a/hw/femu/bbssd/ftl.c +++ b/hw/femu/bbssd/ftl.c @@ -1586,10 +1586,13 @@ static FemuReclaimUnit *select_victim_ru(struct ssd *ssd, uint16_t rgid, /* * Also remove from the global queue (uses the still-valid pos); * the global victim_ru_cnt-- and pos=0 happen at the shared - * cleanup below, so do not touch them here. + * cleanup below, so do not touch them here. With nrg>1 the + * per-RUH queue can hold RUs from any RG, so the global twin + * lives in the victim's OWN RG queue, not the caller's rgid. */ if (victim_ru->pos) { - pqueue_remove(rm->victim_ru_pq, victim_ru); + pqueue_remove(ssd->rg[victim_ru->rgidx].ru_mgmt->victim_ru_pq, + victim_ru); } } } else { @@ -1667,7 +1670,12 @@ static FemuReclaimUnit *select_victim_ru(struct ssd *ssd, uint16_t rgid, victim_ru->pos = 0; victim_ru->ruh_pos = 0; - rm->victim_ru_cnt--; + /* + * Decrement the count on the victim's OWN reclaim group. For every path + * except cross-RG NOISY selection this is the caller's rgid; NOISY can pull + * a victim from another RG, whose global count must be the one adjusted. + */ + ssd->rg[victim_ru->rgidx].ru_mgmt->victim_ru_cnt--; return victim_ru; } @@ -2471,6 +2479,21 @@ static void ssd_trim_fdp_style(FemuCtrl *n, NvmeRequest *req, uint64_t slba, rm->victim_ru_cnt--; mark_ru_free(ssd, v_ru->rgidx, v_ru); } + /* + * GC_GLOBAL_CB keeps its full victims in victim_ru_cb, not + * victim_ru_pq, so drain it too or those RUs leak with a stale + * victim_ru_cnt on trim/format. The two queues are mutually exclusive + * per RG mode (the inactive one is empty here) and victim_ru_cb indexes + * via the same pos field, so this is safe; guard the shared count + * against underflow in case it was already inconsistent. + */ + while ((v_ru = pqueue_peek(rm->victim_ru_cb)) != NULL) { + pqueue_remove(rm->victim_ru_cb, v_ru); + if (rm->victim_ru_cnt > 0) { + rm->victim_ru_cnt--; + } + mark_ru_free(ssd, v_ru->rgidx, v_ru); + } while ((v_ru = QTAILQ_FIRST(&rm->full_ru_list)) != NULL) { QTAILQ_REMOVE(&rm->full_ru_list, v_ru, entry); rm->full_ru_cnt--; From 9d356c6a549d2a1be08806df8177d0f09cd77edc Mon Sep 17 00:00:00 2001 From: Huaicheng Li Date: Fri, 19 Jun 2026 21:05:06 -0400 Subject: [PATCH 2/2] femu/fdp: spread default writes across reclaim groups so nrg>1 is usable 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. --- hw/femu/bbssd/ftl.c | 114 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 98 insertions(+), 16 deletions(-) diff --git a/hw/femu/bbssd/ftl.c b/hw/femu/bbssd/ftl.c index 43bd9f5291..7978d391b2 100644 --- a/hw/femu/bbssd/ftl.c +++ b/hw/femu/bbssd/ftl.c @@ -1152,6 +1152,36 @@ static FemuReclaimUnit *fdp_get_new_ru(struct ssd *ssd, uint16_t rgidx, return new_ru; } +/* + * fdp_get_new_ru_spread - like fdp_get_new_ru but, when the preferred reclaim + * group is exhausted, fall back to any other RG that still has a free RU. + * + * This is for NON-placement (default) writes only. NVMe FDP does not bind a + * default write to a specific RG (the controller chooses), so spreading default + * writes across all RGs is spec-compliant and is what makes the FULL device + * usable when nrg>1. Without it, all default writes funnel to rg[0], which owns + * only tt_nru/nrg of the capacity, so the device wedges at ~1/nrg full with no + * victims yet to GC. Directive (placement) writes must NOT use this -- they are + * pinned to their requested RG via fdp_get_new_ru(). + */ +static FemuReclaimUnit *fdp_get_new_ru_spread(struct ssd *ssd, uint16_t rgidx, + uint16_t ruhid) +{ + FemuReclaimUnit *new_ru = fdp_get_new_ru(ssd, rgidx, ruhid); + if (new_ru) { + return new_ru; + } + /* preferred RG is full; try the others in round-robin order */ + for (uint16_t off = 1; off < (uint16_t)ssd->nrg; off++) { + uint16_t alt = (rgidx + off) % (uint16_t)ssd->nrg; + new_ru = fdp_get_new_ru(ssd, alt, ruhid); + if (new_ru) { + return new_ru; + } + } + return NULL; +} + /* * fdp_get_new_page - get next PPA from an RU's write pointer */ @@ -1182,7 +1212,8 @@ static struct ppa fdp_get_new_page(struct ssd *ssd, FemuReclaimUnit *ru) static FemuReclaimUnit *fdp_advance_ru_pointer(struct ssd *ssd, FemuReclaimGroup *rg, FemuRuHandle *ruh, - FemuReclaimUnit *ru) + FemuReclaimUnit *ru, + bool allow_spread) { struct ssdparams *spp = &ssd->sp; struct ru_mgmt *rm = rg->ru_mgmt; @@ -1241,7 +1272,20 @@ static FemuReclaimUnit *fdp_advance_ru_pointer(struct ssd *ssd, /* allocate a new RU for this RUH cuase ruh->curr_ru is full */ if (ruh != NULL) { check_addr(wpp->blk, spp->blks_per_pl); - new_ru = fdp_get_new_ru(ssd, ru->rgidx, ruh->ruhid); + /* + * Prefer the same RG as the RU that just filled. For the + * default host-write frontier (allow_spread) fall back to any + * other RG with a free RU: with nrg>1 this keeps the whole + * device usable, since a single RG's capacity (tt_nru/nrg) is + * otherwise exhausted during fill -- before overwrites create + * GC victims -- and the device wedges at ~1/nrg. Only the + * future frontier moves RG; written data keeps its RG via its + * PPA. The GC destination frontier passes allow_spread=false + * so GC stays pinned to the victim's RG (correct accounting). + */ + new_ru = allow_spread ? + fdp_get_new_ru_spread(ssd, ru->rgidx, ruh->ruhid) : + fdp_get_new_ru(ssd, ru->rgidx, ruh->ruhid); if (!new_ru) { ftl_err("No free RU for ruh %d: device full - point %s L:%d\n", ruh->ruhid, __FILE__, __LINE__); @@ -1724,20 +1768,26 @@ static void gc_write_page_fdp_style(struct ssd *ssd, struct ppa *old_ppa, * right after the advance. */ + /* + * GC destination frontier stays PINNED to the victim's RG (allow_spread= + * false): a GC write must reclaim within the same reclaim group, and pinning + * guarantees ret_ru->rgidx == dest_ru->rgidx so the rus[dest_ru->rgidx] + * indexing below is correct. + */ if(dest_ruh->ruh_type == NVME_RUHT_PERSISTENTLY_ISOLATED ){ // handle ruh->ru pointer after adv - if( (ret_ru = fdp_advance_ru_pointer(ssd, &ssd->rg[dest_ru->rgidx], dest_ru->ruh, dest_ru)) != dest_ru){ + if( (ret_ru = fdp_advance_ru_pointer(ssd, &ssd->rg[dest_ru->rgidx], dest_ru->ruh, dest_ru, false)) != dest_ru){ dest_ruh->gc_ru = ret_ru; } }else if (dest_ruh->ruh_type == NVME_RUHT_INITIALLY_ISOLATED ){ int gcruh_id = ssd->nruhs-1; ftl_assert( dest_ruh->ruhid == gcruh_id ); - if( (ret_ru = fdp_advance_ru_pointer(ssd, &ssd->rg[dest_ru->rgidx], dest_ruh, dest_ru)) != dest_ru ) { + if( (ret_ru = fdp_advance_ru_pointer(ssd, &ssd->rg[dest_ru->rgidx], dest_ruh, dest_ru, false)) != dest_ru ) { //Do ugly updates ssd->ruhs[gcruh_id].rus[dest_ru->rgidx] = ret_ru; ssd->ruhs[gcruh_id].curr_ru = ret_ru; ssd->ruhs[gcruh_id].ruh->rus[dest_ru->rgidx] = ret_ru->nvme_ru; - } + } } ftl_assert((ret_ru != NULL)); @@ -1978,7 +2028,15 @@ static int do_gc_fdp_style(struct ssd *ssd, uint16_t rgid, uint16_t ruhid, } } - mark_ru_free(ssd, rgid, victim_ru); + /* + * Free the victim into ITS OWN reclaim group, not the caller's rgid. With + * the cross-RG spread fallback a victim can belong to a different RG than + * the GC was invoked for; freeing it into the wrong RG corrupts per-RG + * free_ru_cnt/free_ru_list -- GC then keeps "reclaiming" without ever + * restoring free RUs to the RG under pressure and livelocks on fully-valid + * RUs. mark_ru_free uses ssd->rg[rgid], so pass the victim's rgidx. + */ + mark_ru_free(ssd, victim_ru->rgidx, victim_ru); return 0; } @@ -1990,7 +2048,6 @@ static uint64_t ssd_stream_write(FemuCtrl *n, struct ssd *ssd, { NvmeNamespace *ns = req->ns; struct ssdparams *spp = &ssd->sp; - FemuReclaimGroup *rg; FemuRuHandle *ruh; FemuReclaimUnit *ru; @@ -2007,9 +2064,17 @@ static uint64_t ssd_stream_write(FemuCtrl *n, struct ssd *ssd, uint16_t pid = req->fdp_dspec; uint8_t dtype = req->fdp_dtype; uint16_t ph, rgid, ruhid; + /* + * is_placement: the host issued a valid placement directive selecting a + * specific reclaim group. Such writes MUST stay pinned to that RG (no cross- + * RG spread, no rgid re-sync). Non-placement (default) writes have no RG + * guarantee and may spread across RGs to use the whole nrg>1 device. + */ + bool is_placement = true; if (dtype != NVME_DIRECTIVE_DATA_PLACEMENT || !nvme_parse_pid(ns, pid, &ph, &rgid)) { + is_placement = false; /* generate INVALID_PID event if placement was attempted */ if (dtype == NVME_DIRECTIVE_DATA_PLACEMENT && ssd->n->subsys) { NvmeEnduranceGroup *endgrp = &ssd->n->subsys->endgrp; @@ -2035,7 +2100,6 @@ static uint64_t ssd_stream_write(FemuCtrl *n, struct ssd *ssd, (unsigned)ruhid, (unsigned long)ssd->nruhs); ruhid = 0; } - rg = &ssd->rg[rgid]; ruh = &ssd->ruhs[ruhid]; // FDP_TRACE(ssd, "WRITE lpn=%lu-%lu dtype=%u dspec=0x%x ph=%u " @@ -2056,11 +2120,16 @@ static uint64_t ssd_stream_write(FemuCtrl *n, struct ssd *ssd, //for (int gi = 0; gi < max_fg_gc && !ruh->curr_ru; gi++) { // r = do_gc_fdp_style(ssd, rgid, ruhid, true); // if (r == -1) break; - /* GC may have freed a RU; try to grab it */ - FemuReclaimUnit *fresh = fdp_get_new_ru(ssd, rgid, ruhid); + /* GC may have freed a RU; try to grab it. For default writes spread + * across RGs so a single exhausted RG does not wedge the device when + * nrg>1; placement writes stay pinned to their requested RG. The new + * RU may come from a different RG, so index rus[] by fresh->rgidx. */ + FemuReclaimUnit *fresh = is_placement ? + fdp_get_new_ru(ssd, rgid, ruhid) : + fdp_get_new_ru_spread(ssd, rgid, ruhid); if (fresh) { - ruh->rus[rgid] = fresh; - ruh->ruh->rus[rgid] = fresh->nvme_ru; + ruh->rus[fresh->rgidx] = fresh; + ruh->ruh->rus[fresh->rgidx] = fresh->nvme_ru; ruh->curr_ru = fresh; }else{ ftl_err("NO reclaim Unit. Device is full error\n"); @@ -2072,6 +2141,14 @@ static uint64_t ssd_stream_write(FemuCtrl *n, struct ssd *ssd, return 0; /* return zero latency; write will be retried by GC */ } } + /* + * Re-sync rgid to the active RU's actual RG. For default writes the spread + * fallback may have placed curr_ru in a different RG than requested; for + * placement writes curr_ru stays pinned to the requested RG so this is a + * no-op. Keeps the assert and per-RG bookkeeping (rus[rgid], foreground GC + * target, post-write rotation) consistent either way. + */ + rgid = ruh->curr_ru->rgidx; ru = ruh->rus[rgid]; ru = ruh->curr_ru; ftl_assert(ruh->curr_ru == ruh->rus[rgid]); @@ -2117,12 +2194,17 @@ static uint64_t ssd_stream_write(FemuCtrl *n, struct ssd *ssd, ru->nvme_ru->ruamw--; //TODO ? Does this really decrements page KiB? } - /* advance RU write pointer; may allocate new RU */ - FemuReclaimUnit *ret = fdp_advance_ru_pointer(ssd, rg, ruh, ru); + /* advance RU write pointer; may allocate new RU. Use the RU's OWN RG + * (it can differ from the request's rgid after a spread fallback) so + * the filled RU is classified into the correct RG's victim/full list. + * allow_spread only for default writes; placement writes stay pinned. */ + FemuReclaimUnit *ret = fdp_advance_ru_pointer(ssd, + &ssd->rg[ru->rgidx], ruh, ru, !is_placement); if (ret && ret != ruh->curr_ru) { - ruh->rus[rgid] = ret; + /* the new active RU may have come from a different RG via spread */ + ruh->rus[ret->rgidx] = ret; ruh->curr_ru = ret; - ruh->ruh->rus[rgid] = ret->nvme_ru; + ruh->ruh->rus[ret->rgidx] = ret->nvme_ru; ru = ret; } else if (!ret) { /*