pq/libfreemkv
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

disc/read_error: unify Pass 1 and Pass N error handling

Browse Source 
User's design call after watching the avoidance work prevent a wedge
on the live rip (no wedge events across 6 read errors): "Pass N
and 1 should both be very very similar in recovery. almost identical
just smaller sectors imo in pass n. pause times the same imo as a
failed read is a failed read."

The error-handling code path was already centralized (one
handle_read_error fn, called by both Disc::sweep and Disc::patch).
The TUNING was split — Pass 1 used 5 s inter-error pauses + a
wedge-skip-and-continue policy; Pass N used 1 s pauses + immediate
AbortPass on HARDWARE_ERROR / ILLEGAL_REQUEST. That asymmetry made
Pass N vulnerable to the same wedge that Pass 1's avoidance fixed.

Changes:

1. FAIL_PAUSE_SECS = 5 — single constant, applied uniformly to both
   passes. Dropped PASS_1_FAIL_PAUSE_SECS and POST_FAILURE_PAUSE_SECS
   in favor of one value. CONSECUTIVE_FAIL_LONG_PAUSE_SECS kept as a
   distinct (but currently equal) value for future tuning escalation.

2. HARDWARE_ERROR / ILLEGAL_REQUEST path is now symmetric:
   - Pass 1: JumpAhead WEDGE_JUMP_SECTORS (1 GB) + WEDGE_PAUSE_SECS
     cooldown, mark skipped region NonTrimmed.
   - Pass N: JumpAhead WEDGE_PASS_N_SKIP_SECTORS (64 sectors / 128 KB)
     + WEDGE_PAUSE_SECS cooldown. Pass N's batch=1 means a 1 GB skip
     would abandon the entire current NonTrimmed range; small skip
     moves past the bricked LBA + buffer, outer patch loop picks up
     the next sector.
   - Both share WEDGE_ABORT_THRESHOLD — same 16-skip budget before
     real AbortPass on a permanently stuck drive.

3. wedge_skip / wedge_abort tracing logs now include `pass=1|N`
   so post-mortems can see which pass hit the wedge condition.

Cost analysis:

Pre-reframe worry was "5 s × 5500 NonTrimmed sectors per Pass N
pass × 7 passes = 53 hours." Reality: most NonTrimmed sectors
recover on first or second retry, so most reads are successful and
pay 0 pause. The few that DON'T recover hit the 10-skip budget and
get marked Unreadable — bounded at 10 × 5 s = 50 s per truly-bad
sector. Worst-case Pass N pause overhead on a typical damaged disc
is single-digit minutes, not hours. And it's strictly cheaper than
the alternative (wedge kills the entire multi-pass recovery).

Tests:

- `both_passes_pause_on_failed_read_for_wedge_avoidance` — locks the
  unified pause-tuning policy (was pass_1_pauses_briefly).
- `pass_n_hardware_error_also_skips_not_aborts` — was
  `pass_n_hardware_error_still_aborts`. New behavior verified:
  JumpAhead with WEDGE_PASS_N_SKIP_SECTORS + WEDGE_PAUSE_SECS.
- `pass_n_hardware_error_aborts_after_threshold` — new. Confirms
  Pass N respects the same WEDGE_ABORT_THRESHOLD as Pass 1.
- pass_1_does_not_pause_on_skip is gone (it was the old "Pass 1
  pause=0" assertion, irrelevant after the avoidance work).

Empirical validation: avoidance was already proven on a live rip
tonight — 6 read errors on a damaged disc, sense_family=Medium
throughout, wedge_count=0, Pass 1 continued cleanly past 40%
where it previously died at 48%. This commit extends the same
discipline to Pass N's recovery loop.

Precommit (cargo +1.86 fmt + clippy + test) green.
This commit is contained in:
matthew
2026-05-10 18:04:01 -07:00
parent 01bf3a16db
commit c4c901f073
1 changed files with 112 additions and 81 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/disc/read_error.rs
+111 -80
View File
@@ -294,7 +294,26 @@ pub enum ReadAction {
// Pause budget constants. Tuned from 2026-05-07 BU40N traces showing // Pause budget constants. Tuned from 2026-05-07 BU40N traces showing
// bridge wedges 524 ms after a 5.4-second internal ECC retry. The // bridge wedges 524 ms after a 5.4-second internal ECC retry. The
// post-failure pauses give the drive — and the bridge — time to settle. // post-failure pauses give the drive — and the bridge — time to settle.
const POST_FAILURE_PAUSE_SECS: u64 = 1; /// Pause between a failed read and the next read attempt — applied
/// uniformly to Pass 1 sweep and Pass N patch.
///
/// 2026-05-11 reframe: a failed read is a failed read, regardless of
/// which pass is running. The prior split (1s for Pass N, 5s for Pass
/// 1 via `PASS_1_FAIL_PAUSE_SECS`) was solving an imaginary cost
/// problem — real damaged-disc cases mark <50 MB NonTrimmed, and the
/// extra 5s/error is single-digit minutes per pass, not hours. The
/// cost of NOT pausing — a drive wedge that aborts the entire
/// multi-pass recovery — is much worse.
///
/// The wedge avoidance principle: error → drive ECC retry (5-10s
/// internal) → return → cooldown pause → next read. Same shape
/// everywhere reads can fail.
const FAIL_PAUSE_SECS: u64 = 5;
/// Cooldown when a long streak of failures suggests the drive is
/// stuck in a damage zone and needs MORE breathing room than the
/// standard inter-error pause. Same value as `FAIL_PAUSE_SECS`
/// because empirically 5s is enough; kept as a separate name so the
/// escalation policy is explicit at the call site.
const CONSECUTIVE_FAIL_LONG_PAUSE_SECS: u64 = 5; const CONSECUTIVE_FAIL_LONG_PAUSE_SECS: u64 = 5;
const CONSECUTIVE_FAIL_LONG_PAUSE_THRESHOLD: u64 = 10; const CONSECUTIVE_FAIL_LONG_PAUSE_THRESHOLD: u64 = 10;
const POST_JUMP_EXTRA_PAUSE_SECS: u64 = 2; const POST_JUMP_EXTRA_PAUSE_SECS: u64 = 2;
@@ -303,14 +322,6 @@ const NOT_READY_MAX_RETRIES: u32 = 3;
const BRIDGE_DEGRADATION_PAUSE_SECS: u64 = 15; const BRIDGE_DEGRADATION_PAUSE_SECS: u64 = 15;
const BRIDGE_DEGRADATION_MAX_RETRIES: u32 = 5; const BRIDGE_DEGRADATION_MAX_RETRIES: u32 = 5;
/// Pass 1 inter-error pause. ZERO on the clean path (sweep zooms
/// past damage windows) but applied to every failed read so the
/// drive's firmware gets cool-down time between damage-cluster
/// exposures. Wedge-avoidance change 2026-05-10: pre-fix `pause_secs
/// = 0` on Pass 1 errors caused back-to-back exposures that
/// accumulated firmware wedge state.
const PASS_1_FAIL_PAUSE_SECS: u64 = 5;
/// Base of the damage-jump distance formula: `jump_sectors = /// Base of the damage-jump distance formula: `jump_sectors =
/// JUMP_BASE_SECTORS × batch × jump_multiplier`. Bumped 2026-05-10 /// JUMP_BASE_SECTORS × batch × jump_multiplier`. Bumped 2026-05-10
/// from 256 → 1024 (4×) so the first damage-jump at batch=32 covers /// from 256 → 1024 (4×) so the first damage-jump at batch=32 covers
@@ -360,6 +371,15 @@ const WEDGE_PAUSE_SECS: u64 = 30;
/// a permanently bricked drive. /// a permanently bricked drive.
const WEDGE_ABORT_THRESHOLD: u64 = 16; const WEDGE_ABORT_THRESHOLD: u64 = 16;
/// Pass-N wedge-skip distance. Pass N's batch=1 reads target
/// specific NonTrimmed sectors from Pass 1, so a big 1 GB skip
/// would blow past the current NonTrimmed range and abandon many
/// sectors that might still recover. Use a smaller skip just to
/// move past the bricked LBA + a small buffer — the outer patch
/// loop's next iteration picks up the next sector in the same or
/// next range.
const WEDGE_PASS_N_SKIP_SECTORS: u64 = 64;
/// THE single error-handling entry point. Updates `ctx`, returns the /// THE single error-handling entry point. Updates `ctx`, returns the
/// action the caller must apply. /// action the caller must apply.
/// ///
@@ -479,27 +499,27 @@ pub fn handle_read_error(err: &Error, ctx: &mut ReadCtx) -> ReadAction {
// 4. Hardware error / illegal request — the firmware-wedge family. // 4. Hardware error / illegal request — the firmware-wedge family.
// The drive transitioned into a fast-fail state where it // The drive transitioned into a fast-fail state where it
// rejects reads near the LBA. Two policies: // rejects reads near the LBA. Same response shape for both
// passes (2026-05-11 reframe — error handling is centralized,
// and the wedge is a code-induced state we can avoid via
// pacing + skip):
// //
// Pass 1 sweep (bisect_on_marginal=false): the wedge is // - Pass 1 sweep (bisect_on_marginal=false): jump
// *recoverable* by skipping. Jump a large distance ahead // WEDGE_JUMP_SECTORS (1 GB) ahead, pause WEDGE_PAUSE_SECS,
// (1 GB), pause for cooldown, mark the skipped region // mark skipped region NonTrimmed.
// NonTrimmed so Pass N revisits. Allow up to // - Pass N patch (bisect_on_marginal=true): give up on the
// WEDGE_ABORT_THRESHOLD consecutive wedges before truly // current sector (the granular target), pause for cooldown,
// giving up. This replaces the pre-fix "abort the entire // let the outer patch loop move to the next NonTrimmed
// rip on first wedge" behavior that caused 48%-and-die // range. Implemented as a small JumpAhead so the same code
// failures on discs with one bad cluster. // path serves both — Pass N's batch=1 means JumpAhead by
// WEDGE_PASS_N_SKIP_SECTORS effectively skips just this
// sector and a small buffer (gives the drive room to
// recover before the next per-sector attempt).
// //
// Pass N patch (bisect_on_marginal=true): Pass N's job IS // Both paths share the WEDGE_ABORT_THRESHOLD budget — only
// single-sector recovery; a wedge means the drive won't // AbortPass after N consecutive wedges with no successful
// give us the specific sectors we asked for. Skipping // read in between.
// doesn't help here. Abort and let autorip decide whether
// to retry, eject, or surface the failure to the user.
if sense_key == scsi::SENSE_KEY_HARDWARE_ERROR || sense_key == scsi::SENSE_KEY_ILLEGAL_REQUEST { if sense_key == scsi::SENSE_KEY_HARDWARE_ERROR || sense_key == scsi::SENSE_KEY_ILLEGAL_REQUEST {
if ctx.bisect_on_marginal {
return ReadAction::AbortPass;
}
// Pass 1 wedge-skip path.
if !ctx.bisecting { if !ctx.bisecting {
ctx.wedge_count += 1; ctx.wedge_count += 1;
} }
@@ -509,21 +529,28 @@ pub fn handle_read_error(err: &Error, ctx: &mut ReadCtx) -> ReadAction {
phase = "wedge_abort", phase = "wedge_abort",
wedge_count = ctx.wedge_count, wedge_count = ctx.wedge_count,
threshold = WEDGE_ABORT_THRESHOLD, threshold = WEDGE_ABORT_THRESHOLD,
"Pass 1 wedge-skip exhausted — drive appears permanently stuck" pass = if ctx.bisect_on_marginal { "N" } else { "1" },
"wedge-skip exhausted — drive appears permanently stuck"
); );
return ReadAction::AbortPass; return ReadAction::AbortPass;
} }
let jump_sectors = if ctx.bisect_on_marginal {
WEDGE_PASS_N_SKIP_SECTORS
} else {
WEDGE_JUMP_SECTORS
};
tracing::warn!( tracing::warn!(
target: "freemkv::disc", target: "freemkv::disc",
phase = "wedge_skip", phase = "wedge_skip",
pass = if ctx.bisect_on_marginal { "N" } else { "1" },
wedge_count = ctx.wedge_count, wedge_count = ctx.wedge_count,
jump_sectors = WEDGE_JUMP_SECTORS, jump_sectors,
pause_secs = WEDGE_PAUSE_SECS, pause_secs = WEDGE_PAUSE_SECS,
"Pass 1 wedge detected — jumping ahead and pausing for drive cooldown" "wedge detected — skipping ahead and pausing for drive cooldown"
); );
ctx.jumps_taken += 1; ctx.jumps_taken += 1;
return ReadAction::JumpAhead { return ReadAction::JumpAhead {
sectors: WEDGE_JUMP_SECTORS, sectors: jump_sectors,
pause_secs: WEDGE_PAUSE_SECS, pause_secs: WEDGE_PAUSE_SECS,
}; };
} }
@@ -572,35 +599,15 @@ pub fn handle_read_error(err: &Error, ctx: &mut ReadCtx) -> ReadAction {
bad_count * 100 / ctx.damage_window.len() bad_count * 100 / ctx.damage_window.len()
}; };
// Inter-error pause on Pass 1 — wedge avoidance. // Inter-error pause — wedge avoidance, applied uniformly across
// // Pass 1 and Pass N. A failed read is a failed read; same
// Pre-2026-05-10 Pass 1 ran `pause_secs = 0` on all errors so // cool-down regardless of which pass is calling. The escalation
// sweep would zoom past damage zones in seconds instead of // arm (LONG_PAUSE after a long streak) currently resolves to the
// minutes. Empirically on the BU40N this caused firmware-wedge // same value but is kept as a separate branch for future tuning.
// events: each read failure leaves residual state in the drive's let pause_secs = if ctx.consecutive_failures >= CONSECUTIVE_FAIL_LONG_PAUSE_THRESHOLD {
// firmware, and back-to-back errors without cooldown accumulate
// toward a wedge threshold. We hit wedge on Dune Pt 2 after 5
// errors in 43 s spread over a 140 MB damage cluster — even
// though wall-clock pacing was slow, the drive had no breathing
// room between exposures.
//
// New policy: Pass 1 keeps `pause = 0` on the CLEAN path
// (successful reads zoom past damage windows fine), but every
// failed read takes a PASS_1_FAIL_PAUSE_SECS pause before the
// next read. Cost: ~5 s extra per scattered failure, ~30-60 s
// total in a damage cluster — trivial compared to the alternative
// of crashing the whole rip at 48%. Long failure streaks still
// escalate via CONSECUTIVE_FAIL_LONG_PAUSE.
let pause_secs = if !ctx.bisect_on_marginal {
if ctx.consecutive_failures >= CONSECUTIVE_FAIL_LONG_PAUSE_THRESHOLD {
CONSECUTIVE_FAIL_LONG_PAUSE_SECS CONSECUTIVE_FAIL_LONG_PAUSE_SECS
} else { } else {
PASS_1_FAIL_PAUSE_SECS FAIL_PAUSE_SECS
}
} else if ctx.consecutive_failures >= CONSECUTIVE_FAIL_LONG_PAUSE_THRESHOLD {
CONSECUTIVE_FAIL_LONG_PAUSE_SECS
} else {
POST_FAILURE_PAUSE_SECS
}; };
// 7. Damage-jump: too many failures → skip ahead by an escalating // 7. Damage-jump: too many failures → skip ahead by an escalating
@@ -868,13 +875,40 @@ mod tests {
} }
#[test] #[test]
fn pass_n_hardware_error_still_aborts() { fn pass_n_hardware_error_also_skips_not_aborts() {
// Pass N (bisect_on_marginal=true) keeps the original // 2026-05-11 reframe: error handling is centralized, the
// AbortPass behavior — single-sector recovery can't make // wedge is a code-induced state, and the avoidance principle
// progress through a wedge, so the right answer is to bail // (skip + pause + continue) applies to Pass N too. Previously
// and let the outer layer decide. // Pass N AbortPass'd on first wedge — same fatal-at-48% bug
// Pass 1 had pre-fix. Now Pass N gets a smaller skip
// (WEDGE_PASS_N_SKIP_SECTORS, not the 1 GB Pass 1 jump)
// because Pass N's job IS to revisit specific NonTrimmed
// ranges; over-skipping abandons recoverable sectors.
let mut ctx = ReadCtx::for_patch(1); let mut ctx = ReadCtx::for_patch(1);
let action = handle_read_error(&hardware_err(), &mut ctx); let action = handle_read_error(&hardware_err(), &mut ctx);
match action {
ReadAction::JumpAhead {
sectors,
pause_secs,
} => {
assert_eq!(sectors, WEDGE_PASS_N_SKIP_SECTORS);
assert_eq!(pause_secs, WEDGE_PAUSE_SECS);
}
other => panic!("expected JumpAhead, got {other:?}"),
}
assert_eq!(ctx.wedge_count, 1);
}
#[test]
fn pass_n_hardware_error_aborts_after_threshold() {
// Same threshold as Pass 1 — after WEDGE_ABORT_THRESHOLD
// consecutive wedges with no good read in between, give up.
let mut ctx = ReadCtx::for_patch(1);
for _ in 0..WEDGE_ABORT_THRESHOLD - 1 {
let action = handle_read_error(&hardware_err(), &mut ctx);
assert!(matches!(action, ReadAction::JumpAhead { .. }));
}
let action = handle_read_error(&hardware_err(), &mut ctx);
assert_eq!(action, ReadAction::AbortPass); assert_eq!(action, ReadAction::AbortPass);
} }
@@ -911,24 +945,21 @@ mod tests {
} }
#[test] #[test]
fn pass_1_pauses_briefly_on_skip_for_wedge_avoidance() { fn both_passes_pause_on_failed_read_for_wedge_avoidance() {
// Pre-2026-05-10 Pass 1 ran pause_secs=0 on all errors (zoom // 2026-05-11 reframe: a failed read is a failed read. Same
// past damage zones in seconds). That caused firmware wedges // FAIL_PAUSE_SECS for both Pass 1 (sweep) and Pass N (patch).
// on the BU40N — back-to-back errors with no cooldown built // Pre-reframe Pass 1 had its own PASS_1_FAIL_PAUSE_SECS=5
// up firmware state until the drive entered the wedge fast- // and Pass N had POST_FAILURE_PAUSE_SECS=1 — the asymmetry
// fail mode. New policy: a brief inter-error pause (5 s) on // is gone. The wedge avoidance is a centralized policy now.
// Pass 1 to give the drive's firmware time to settle between for mut ctx in [ReadCtx::for_sweep(32), ReadCtx::for_patch(1)] {
// damage-zone exposures. Successful reads remain zero-pause
// — only errors cost time, and only a few seconds per
// scattered failure. Trivial cost compared to crashing the
// whole rip at 48%.
let mut ctx = ReadCtx::for_sweep(32);
let action = handle_read_error(&medium_err(), &mut ctx); let action = handle_read_error(&medium_err(), &mut ctx);
match action { let pause = match action {
ReadAction::SkipBlock { pause_secs } => { ReadAction::SkipBlock { pause_secs } => pause_secs,
assert_eq!(pause_secs, PASS_1_FAIL_PAUSE_SECS); ReadAction::JumpAhead { pause_secs, .. } => pause_secs,
} ReadAction::Bisect => continue,
other => panic!("expected SkipBlock, got {other:?}"), other => panic!("expected pausing action, got {other:?}"),
};
assert_eq!(pause, FAIL_PAUSE_SECS);
} }
} }