v0.18.7: Pass 1 fast-skip, defer recovery to Pass N

Pass 1 sweep was grinding through damage zones because the marginal-
media handler returned `Bisect` for every failed 32-sector batch —
forcing 32 single-sector reads per bad block at ~5s each on a real
BU40N-vs-Dune-Pt-2 trace. AND the JumpAhead trigger required a 16-
block damage window to fill before firing, so entry into a
contiguous damage zone took ~40 minutes of grinding before the
first jump fired. Architecturally wrong: Pass 1's job is "fast and
accurate, get the most data in the shortest time." Bisection +
recovery is Pass N's purpose-built role.

ReadCtx now carries two new fields:
  - `consecutive_outer_failures: u64` — outer-batch failures since
    last outer success. Bisect inner failures don't count.
  - `bisect_on_marginal: bool` — whether to return Bisect on a
    marginal-media batch failure.
  - `fast_jump_threshold: u64` — outer-failures count that triggers
    JumpAhead before the damage window has filled.

`for_sweep` (Pass 1) sets `bisect_on_marginal=false`,
`fast_jump_threshold=4`, and zeroes the post-failure pause. Failed
batches become SkipBlock → whole block NonTrimmed → advance, no
sleep. After 4 consecutive outer failures: JumpAhead with the
existing escalating multiplier.

`for_patch` (Pass N) sets `bisect_on_marginal=true`,
`fast_jump_threshold=u64::MAX`, keeps the original cooldown pauses.
Pass N's whole reason to exist is to grind on bad ranges with
proper recovery semantics — single-sector reads, 60s recovery
timeout, retry budget, escalating skip — and that's unchanged.

`on_success` resets `consecutive_outer_failures` only when not
bisecting, so a good single-sector read inside Pass N's bisect
doesn't pretend we've escaped the damaged batch.

Tests:
  - `pass_n_marginal_with_batch_gt_1_bisects` — Pass N still bisects.
  - `pass_1_marginal_skips_instead_of_bisecting` — Pass 1 doesn't.
  - `pass_1_jumps_after_4_consecutive_outer_failures` — fast-entry.
  - `pass_n_does_not_fast_jump` — fast-entry is Pass-1-only.
  - `outer_success_resets_consecutive_outer_failures` — counter reset.
  - `bisect_inner_success_does_not_reset_outer_counter` — semantics.
  - `pass_1_does_not_pause_on_skip` — explicit zero-pause contract.
  - `long_failure_streak_extends_pause_on_pass_n` — Pass N still
    extends pauses on long failure streaks (renamed from the old
    sweep-based test).

Integration test `test_disc_copy_marks_failed_ecc_blocks_as_nontrimmed`
updated: it used to assert Pass 1 recovers all sectors via bisect
(bytes_good=total). New contract: Pass 1 marks NonTrimmed; Pass N
recovers. Test now asserts Pass-1-only outcome (bytes_pending=total,
complete=false) consistent with the redesign.

Real-world impact on the user's BU40N + Dune Pt 2 trace from this
session: a damage zone that was on track to take ~40 minutes of
Pass-1 grinding will now jump in ~20 seconds. Pass N still has the
full 7-pass recovery budget to revisit those NonTrimmed ranges.
This commit is contained in:
MattJackson
2026-05-10 12:50:12 -07:00
parent ef5487e5a5
commit 3244fdd683
3 changed files with 222 additions and 27 deletions
+1 -1
View File
@@ -1,6 +1,6 @@
[package] [package]
name = "libfreemkv" name = "libfreemkv"
version = "0.18.6" version = "0.18.7"
edition = "2024" edition = "2024"
rust-version = "1.86" rust-version = "1.86"
license = "AGPL-3.0-only" license = "AGPL-3.0-only"
+204 -17
View File
@@ -25,11 +25,25 @@ pub struct ReadCtx {
/// Failed reads since the last success. Resets to 0 on success. /// Failed reads since the last success. Resets to 0 on success.
/// Drives long-pause escalation on persistent failure. /// Drives long-pause escalation on persistent failure.
pub consecutive_failures: u64, pub consecutive_failures: u64,
/// Failed OUTER batch reads since the last outer success — bisect
/// inner-sector failures are NOT counted here. Drives the
/// fast-entry damage-jump on Pass 1 (skip the disc-level grind
/// once we're clearly in a damaged region; Pass N will recover
/// the actual sectors). Reset on outer success.
pub consecutive_outer_failures: u64,
/// Sliding window of recent read outcomes (true=ok, false=fail). /// Sliding window of recent read outcomes (true=ok, false=fail).
/// Capped at `damage_window_max`. Drives damage-jump decisions. /// Capped at `damage_window_max`. Drives damage-jump decisions.
pub damage_window: Vec<bool>, pub damage_window: Vec<bool>,
pub damage_window_max: usize, pub damage_window_max: usize,
pub damage_threshold_pct: usize, pub damage_threshold_pct: usize,
/// Trigger a damage-jump after this many consecutive outer-batch
/// failures, even when the damage_window isn't full yet. Pass 1
/// uses a small value (4) so we don't spend ~40 minutes grinding
/// to fill a 16-block window before the first jump on a damage
/// zone we entered cleanly. Pass N uses a larger value (or
/// disables this — see `bisect_on_marginal`) because Pass N's
/// whole job IS to grind on the bad ranges.
pub fast_jump_threshold: u64,
/// Multiplier applied to damage-jump distance. Doubles each jump, /// Multiplier applied to damage-jump distance. Doubles each jump,
/// resets to 1 after `damage_window_max` consecutive good reads. /// resets to 1 after `damage_window_max` consecutive good reads.
pub jump_multiplier: u64, pub jump_multiplier: u64,
@@ -43,38 +57,64 @@ pub struct ReadCtx {
/// failed batch, so the handler doesn't recursively request another /// failed batch, so the handler doesn't recursively request another
/// bisect on the inner-sector failures. /// bisect on the inner-sector failures.
pub bisecting: bool, pub bisecting: bool,
/// Whether to return `Bisect` on a marginal-media batch failure.
/// Pass 1 sweep sets this false: a failed batch becomes
/// SkipBlock (mark the whole 32-sector ECC block NonTrimmed,
/// advance, let Pass N recover the salvageable sectors with
/// proper recovery semantics). Pass N sets this true: bisection
/// is its core job, and it has the right tools (single-sector
/// reads, 60s recovery timeout, retry budget, escalating skip).
pub bisect_on_marginal: bool,
} }
impl ReadCtx { impl ReadCtx {
/// Initial context for a Pass 1 sweep with the documented constants. /// Initial context for a Pass 1 sweep. The job is "fast and
/// accurate, get the most data in the shortest time" — Pass N
/// is the one that grinds on the bad ranges. So bisect-on-
/// marginal is OFF (failed batches become SkipBlock; whole 32-
/// sector blocks marked NonTrimmed for Pass N to revisit), and
/// the damage-jump fast-path triggers after just 4 consecutive
/// outer-batch failures.
pub fn for_sweep(batch: u16) -> Self { pub fn for_sweep(batch: u16) -> Self {
Self { Self {
batch, batch,
consecutive_good: 0, consecutive_good: 0,
consecutive_failures: 0, consecutive_failures: 0,
consecutive_outer_failures: 0,
damage_window: Vec::with_capacity(16), damage_window: Vec::with_capacity(16),
damage_window_max: 16, damage_window_max: 16,
damage_threshold_pct: 12, damage_threshold_pct: 12,
fast_jump_threshold: 4,
jump_multiplier: 1, jump_multiplier: 1,
not_ready_retries: 0, not_ready_retries: 0,
bridge_degradation_count: 0, bridge_degradation_count: 0,
bisecting: false, bisecting: false,
bisect_on_marginal: false,
} }
} }
/// Initial context for a Pass 2-N patch. /// Initial context for a Pass 2-N patch. Pass N's whole reason to
/// exist is to recover sectors Pass 1 skipped — bisection on
/// marginal media is part of the job, and the fast-jump
/// threshold is loose so we don't bail too early on a range that
/// has scattered good sectors mixed in.
pub fn for_patch(batch: u16) -> Self { pub fn for_patch(batch: u16) -> Self {
Self { Self {
batch, batch,
consecutive_good: 0, consecutive_good: 0,
consecutive_failures: 0, consecutive_failures: 0,
consecutive_outer_failures: 0,
damage_window: Vec::with_capacity(16), damage_window: Vec::with_capacity(16),
damage_window_max: 16, damage_window_max: 16,
damage_threshold_pct: 12, damage_threshold_pct: 12,
// Pass N is allowed to grind: window-based jump only,
// matching the historical behaviour for patch passes.
fast_jump_threshold: u64::MAX,
jump_multiplier: 1, jump_multiplier: 1,
not_ready_retries: 0, not_ready_retries: 0,
bridge_degradation_count: 0, bridge_degradation_count: 0,
bisecting: false, bisecting: false,
bisect_on_marginal: true,
} }
} }
@@ -83,6 +123,12 @@ impl ReadCtx {
self.consecutive_good += 1; self.consecutive_good += 1;
self.consecutive_failures = 0; self.consecutive_failures = 0;
self.not_ready_retries = 0; self.not_ready_retries = 0;
// Outer-success only: a good single-sector read inside a
// bisect doesn't mean we've left the damaged batch. Only an
// outer-batch success resets the outer-failure counter.
if !self.bisecting {
self.consecutive_outer_failures = 0;
}
self.damage_window.push(true); self.damage_window.push(true);
if self.damage_window.len() > self.damage_window_max { if self.damage_window.len() > self.damage_window_max {
self.damage_window.remove(0); self.damage_window.remove(0);
@@ -137,6 +183,13 @@ const BRIDGE_DEGRADATION_MAX_RETRIES: u32 = 5;
pub fn handle_read_error(err: &Error, ctx: &mut ReadCtx) -> ReadAction { pub fn handle_read_error(err: &Error, ctx: &mut ReadCtx) -> ReadAction {
ctx.consecutive_failures += 1; ctx.consecutive_failures += 1;
ctx.consecutive_good = 0; ctx.consecutive_good = 0;
// Outer-failure counter — only OUTER batch failures count toward
// the fast-jump trigger. Bisect inner failures are part of
// recovering an already-failed batch and don't represent
// independent damage signal.
if !ctx.bisecting {
ctx.consecutive_outer_failures += 1;
}
tracing::warn!( tracing::warn!(
target: "freemkv::disc", target: "freemkv::disc",
@@ -198,11 +251,20 @@ pub fn handle_read_error(err: &Error, ctx: &mut ReadCtx) -> ReadAction {
// individually. Bisect into single-sector reads (gentler on the // individually. Bisect into single-sector reads (gentler on the
// bridge too — shorter SCSI transactions). Avoid recursive // bridge too — shorter SCSI transactions). Avoid recursive
// bisect. // bisect.
//
// Pass 1 sweep sets `bisect_on_marginal=false` to skip this:
// its job is "fast and accurate, get the most data in the
// shortest time." Pass N is purpose-built to recover
// individual sectors with proper recovery semantics, and Pass
// 1 grinding through 32-sector bisects costs ~2.5 min per bad
// block AND fills the damage window slower than it should.
// Whole-block NonTrimmed → SkipBlock → advance → Pass N
// revisits.
let is_marginal = matches!( let is_marginal = matches!(
sense_key, sense_key,
scsi::SENSE_KEY_MEDIUM_ERROR | scsi::SENSE_KEY_ABORTED_COMMAND scsi::SENSE_KEY_MEDIUM_ERROR | scsi::SENSE_KEY_ABORTED_COMMAND
); );
if is_marginal && ctx.batch > 1 && !ctx.bisecting { if is_marginal && ctx.batch > 1 && !ctx.bisecting && ctx.bisect_on_marginal {
return ReadAction::Bisect; return ReadAction::Bisect;
} }
@@ -228,22 +290,49 @@ 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()
}; };
let pause_secs = if ctx.consecutive_failures >= CONSECUTIVE_FAIL_LONG_PAUSE_THRESHOLD { // Pass 1's job is to get to the end of the disc fast. Inter-
// block pauses help the drive cool down on Pass N (gentle
// recovery on bad ranges) but on Pass 1 they just turn a 30 s
// damage zone into 30 minutes of dead-air sleep. Pass N
// (`bisect_on_marginal=true`) keeps the original cooldown logic.
let pause_secs = if !ctx.bisect_on_marginal {
0
} else if ctx.consecutive_failures >= CONSECUTIVE_FAIL_LONG_PAUSE_THRESHOLD {
CONSECUTIVE_FAIL_LONG_PAUSE_SECS CONSECUTIVE_FAIL_LONG_PAUSE_SECS
} else { } else {
POST_FAILURE_PAUSE_SECS POST_FAILURE_PAUSE_SECS
}; };
// 7. Damage-jump: too many failures in window → skip ahead by an // 7. Damage-jump: too many failures → skip ahead by an escalating
// escalating gap. Multiplier capped so we can't accidentally // gap. Multiplier capped so we can't accidentally skip the
// skip the entire rest of the disc (observed 2026-05-07: a // entire rest of the disc (observed 2026-05-07: a saturated
// saturated multiplier produced a 56 GB jump). Saturating // multiplier produced a 56 GB jump). Saturating arithmetic on
// arithmetic on the sector calc as defence in depth. // the sector calc as defence in depth.
//
// Two triggers, evaluated in order:
//
// a. **Fast-entry** — `consecutive_outer_failures >= fast_jump_threshold`.
// Fires on Pass 1 (threshold=4) so we don't spend ~40 min
// grinding to fill a 16-block damage window before the
// first jump on a damage zone we entered cleanly. Doesn't
// fire on Pass N (threshold=u64::MAX).
//
// b. **Window-based** — original behaviour: 12% bad in a
// sliding window of 16 outer reads. Pass N's only path,
// and Pass 1's fallback if the failures are scattered
// enough that we don't hit the consecutive threshold.
const MAX_JUMP_MULTIPLIER: u64 = 64; // 64 × 256 × batch sectors const MAX_JUMP_MULTIPLIER: u64 = 64; // 64 × 256 × batch sectors
if ctx.damage_window.len() >= ctx.damage_window_max && bad_pct >= ctx.damage_threshold_pct { let fast_trigger = !ctx.bisecting && ctx.consecutive_outer_failures >= ctx.fast_jump_threshold;
let window_trigger =
ctx.damage_window.len() >= ctx.damage_window_max && bad_pct >= ctx.damage_threshold_pct;
if fast_trigger || window_trigger {
let mult = ctx.jump_multiplier.min(MAX_JUMP_MULTIPLIER); let mult = ctx.jump_multiplier.min(MAX_JUMP_MULTIPLIER);
let sectors = 256u64.saturating_mul(ctx.batch as u64).saturating_mul(mult); let sectors = 256u64.saturating_mul(ctx.batch as u64).saturating_mul(mult);
ctx.jump_multiplier = (ctx.jump_multiplier.saturating_mul(2)).min(MAX_JUMP_MULTIPLIER); ctx.jump_multiplier = (ctx.jump_multiplier.saturating_mul(2)).min(MAX_JUMP_MULTIPLIER);
// Reset the outer-failure counter so a long damaged region
// doesn't keep firing fast-jump every read after the initial
// jump fired. The window-based trigger handles further jumps.
ctx.consecutive_outer_failures = 0;
return ReadAction::JumpAhead { return ReadAction::JumpAhead {
sectors, sectors,
pause_secs: pause_secs + POST_JUMP_EXTRA_PAUSE_SECS, pause_secs: pause_secs + POST_JUMP_EXTRA_PAUSE_SECS,
@@ -286,15 +375,28 @@ mod tests {
} }
#[test] #[test]
fn medium_error_with_batch_gt_1_bisects() { fn pass_n_marginal_with_batch_gt_1_bisects() {
let mut ctx = ReadCtx::for_sweep(32); let mut ctx = ReadCtx::for_patch(32);
let action = handle_read_error(&medium_err(), &mut ctx); let action = handle_read_error(&medium_err(), &mut ctx);
assert_eq!(action, ReadAction::Bisect); assert_eq!(action, ReadAction::Bisect);
} }
#[test]
fn pass_1_marginal_skips_instead_of_bisecting() {
// Pass 1's job is "fast and accurate" — leave bisection to
// Pass N. A failed batch becomes SkipBlock (whole 32-sector
// block marked NonTrimmed for Pass N to revisit).
let mut ctx = ReadCtx::for_sweep(32);
let action = handle_read_error(&medium_err(), &mut ctx);
match action {
ReadAction::SkipBlock { .. } => {}
other => panic!("expected SkipBlock for Pass 1, got {other:?}"),
}
}
#[test] #[test]
fn medium_error_with_batch_1_skips() { fn medium_error_with_batch_1_skips() {
let mut ctx = ReadCtx::for_sweep(1); let mut ctx = ReadCtx::for_patch(1);
let action = handle_read_error(&medium_err(), &mut ctx); let action = handle_read_error(&medium_err(), &mut ctx);
match action { match action {
ReadAction::SkipBlock { pause_secs } => assert!(pause_secs >= 1), ReadAction::SkipBlock { pause_secs } => assert!(pause_secs >= 1),
@@ -304,7 +406,7 @@ mod tests {
#[test] #[test]
fn medium_error_while_bisecting_does_not_recurse() { fn medium_error_while_bisecting_does_not_recurse() {
let mut ctx = ReadCtx::for_sweep(32); let mut ctx = ReadCtx::for_patch(32);
ctx.bisecting = true; ctx.bisecting = true;
let action = handle_read_error(&medium_err(), &mut ctx); let action = handle_read_error(&medium_err(), &mut ctx);
match action { match action {
@@ -313,6 +415,75 @@ mod tests {
} }
} }
#[test]
fn pass_1_jumps_after_4_consecutive_outer_failures() {
// The fast-entry trigger: Pass 1 should JumpAhead after 4
// consecutive outer-batch failures, BEFORE the 16-block
// damage window has filled. Otherwise we spend ~40 minutes
// of bisecting/grinding to fill the window before the first
// jump on a damage zone we entered cleanly.
let mut ctx = ReadCtx::for_sweep(32);
// First three should NOT jump (still under threshold of 4).
for _ in 0..3 {
let a = handle_read_error(&medium_err(), &mut ctx);
assert!(
!matches!(a, ReadAction::JumpAhead { .. }),
"should not jump until 4 consecutive outer failures"
);
}
// Fourth should jump.
let a = handle_read_error(&medium_err(), &mut ctx);
assert!(
matches!(a, ReadAction::JumpAhead { .. }),
"expected JumpAhead at 4th consecutive outer failure, got {a:?}"
);
}
#[test]
fn pass_n_does_not_fast_jump() {
// Pass N's whole reason to exist is to grind on bad ranges.
// It should NOT bail after 4 consecutive failures the way
// Pass 1 does — it bisects and skips with proper recovery.
let mut ctx = ReadCtx::for_patch(32);
for _ in 0..4 {
let a = handle_read_error(&medium_err(), &mut ctx);
assert!(
!matches!(a, ReadAction::JumpAhead { .. }),
"Pass N must not fast-jump; got {a:?}"
);
}
}
#[test]
fn outer_success_resets_consecutive_outer_failures() {
let mut ctx = ReadCtx::for_sweep(32);
for _ in 0..3 {
handle_read_error(&medium_err(), &mut ctx);
}
assert_eq!(ctx.consecutive_outer_failures, 3);
// An outer-success (bisecting=false) should reset the counter.
ctx.bisecting = false;
ctx.on_success();
assert_eq!(ctx.consecutive_outer_failures, 0);
}
#[test]
fn bisect_inner_success_does_not_reset_outer_counter() {
let mut ctx = ReadCtx::for_patch(32);
for _ in 0..3 {
handle_read_error(&medium_err(), &mut ctx);
}
assert_eq!(ctx.consecutive_outer_failures, 3);
// A successful inner-sector read during bisect is not the
// same as escaping the bad outer batch.
ctx.bisecting = true;
ctx.on_success();
assert_eq!(
ctx.consecutive_outer_failures, 3,
"bisect inner success must not reset outer-failure counter"
);
}
#[test] #[test]
fn hardware_error_aborts() { fn hardware_error_aborts() {
let mut ctx = ReadCtx::for_sweep(32); let mut ctx = ReadCtx::for_sweep(32);
@@ -321,12 +492,15 @@ mod tests {
} }
#[test] #[test]
fn long_failure_streak_extends_pause() { fn long_failure_streak_extends_pause_on_pass_n() {
let mut ctx = ReadCtx::for_sweep(1); // Pass N keeps the cooldown behaviour: after many consecutive
// failures, pauses extend to give the drive time to recover.
// Pass 1 explicitly does NOT pause — see
// `pass_1_does_not_pause_on_skip` below.
let mut ctx = ReadCtx::for_patch(1);
for _ in 0..15 { for _ in 0..15 {
handle_read_error(&medium_err(), &mut ctx); handle_read_error(&medium_err(), &mut ctx);
} }
// After many consecutive failures we should be in the long-pause regime
let final_action = handle_read_error(&medium_err(), &mut ctx); let final_action = handle_read_error(&medium_err(), &mut ctx);
match final_action { match final_action {
ReadAction::SkipBlock { pause_secs } => { ReadAction::SkipBlock { pause_secs } => {
@@ -339,6 +513,19 @@ mod tests {
} }
} }
#[test]
fn pass_1_does_not_pause_on_skip() {
// Pass 1 must zoom — a damage zone is Pass N's problem to
// recover from. Sleeping between failed batches turned a 30s
// damaged region into a 30-minute Pass-1 grind on real discs.
let mut ctx = ReadCtx::for_sweep(32);
let action = handle_read_error(&medium_err(), &mut ctx);
match action {
ReadAction::SkipBlock { pause_secs } => assert_eq!(pause_secs, 0),
other => panic!("expected SkipBlock, got {other:?}"),
}
}
#[test] #[test]
fn damage_window_fills_then_jumps() { fn damage_window_fills_then_jumps() {
let mut ctx = ReadCtx::for_sweep(1); let mut ctx = ReadCtx::for_sweep(1);
+17 -9
View File
@@ -624,19 +624,27 @@ fn test_disc_copy_marks_failed_ecc_blocks_as_nontrimmed() {
let _ = std::fs::remove_file(&iso_path); let _ = std::fs::remove_file(&iso_path);
let _ = std::fs::remove_file(libfreemkv::disc::mapfile_path_for(&iso_path)); let _ = std::fs::remove_file(libfreemkv::disc::mapfile_path_for(&iso_path));
// Pass 1 reads every batch at bpt=32 (no batch reduction, no skip-ahead). // Pass 1's job is "fast and accurate, get the most data in the
// BlockSizeFailingReader fails on multi-sector reads but succeeds on single-sector. // shortest time." It no longer bisects on marginal media — that's
// Bridge degradation handling retries failed batches as individual sectors, // Pass N's purpose-built role. So a BlockSizeFailingReader that
// so all data is recovered as Finished. bytes_good == total_bytes is correct. // fails on multi-sector reads and succeeds on single-sector
// results in: every batch fails → SkipBlock → whole 32-sector
// ECC block marked NonTrimmed → Pass N (Disc::patch) revisits and
// recovers via single-sector reads with proper recovery semantics.
//
// Pass 1 alone:
assert_eq!( assert_eq!(
result.bytes_good, total_bytes, result.bytes_good, 0,
"Pass 1 at bpt=32 recovers all sectors via single-sector retry on MEDIUM_ERROR" "Pass 1 doesn't bisect on marginal media — failed batches become NonTrimmed for Pass N to revisit"
); );
assert_eq!( assert_eq!(
result.bytes_pending, 0, result.bytes_pending, total_bytes,
"no pending sectors after full recovery" "every sector is NonTrimmed (pending) after Pass 1, awaiting Pass N"
);
assert!(
!result.complete,
"complete=false because NonTrimmed regions remain (Pass N's work)"
); );
assert!(result.complete, "complete=true when all sectors recovered");
} }
// ── 9. PassProgress carries separate unreadable vs pending byte counts ───── // ── 9. PassProgress carries separate unreadable vs pending byte counts ─────