From 987e26e44d96a7b95f3b5d5e14fd4fee11adf828 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Wed, 24 Jun 2026 14:58:32 -0700 Subject: [PATCH] libfreemkv: single-pass disc->MKV recovers marginal sectors before giving up Single-pass disc->MKV has no Pass N, so its read bottom-out now issues one bounded recovery read (recovery=true, ~60s ECC) before skipping or aborting, matching the multipass patch. Fixes a transient/marginal sector surfaced as a read failure direct-to-MKV while multipass recovered it. One read, not a loop (hard rule #2); recovered data is used so no bogus-status hole reopens. --- src/mux/disc.rs | 57 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 13 deletions(-) diff --git a/src/mux/disc.rs b/src/mux/disc.rs index dd491ef..fe31fca 100644 --- a/src/mux/disc.rs +++ b/src/mux/disc.rs @@ -448,11 +448,47 @@ impl DiscStream { if (sectors as u32) <= align { // Bottomed out at one unit (AACS) / one sector (CSS) / the - // extent tail. Skip the WHOLE failed unit or bail. Zero-filling - // and advancing by the full unit keeps current_offset - // unit-aligned, so the next read still begins on a real AACS - // unit boundary (a 1-sector skip here would desync the rest of - // the title — the bug this guards). + // extent tail. This is single-pass disc→MKV, which has NO Pass N + // to come back and recover later — so before we skip or bail, + // give the drive its full ECC recovery budget ONCE + // (`recovery=true` → READ_RECOVERY_TIMEOUT_MS, ~60s), exactly as + // the multipass patch does on its bad ranges. A single bounded + // read, never a loop (hard rule #2: tight retry loops on one LBA + // push the BU40N into fast-fail). On success we USE the recovered + // data, so the old "transient retry returns a bogus status for + // readable data" hole cannot reopen; the earlier 10s-timeout read + // gave the drive no chance to recover a marginal sector that a + // 60s ECC read can. + tracing::debug!( + target: "mux", + "fill_extents last-chance recovery read at LBA {} ({} sectors, 60s ECC)", + lba, + sectors + ); + let rec = + self.reader + .read_sectors(lba, sectors, &mut self.read_buf[..bytes], true); + if let Ok(&got) = rec.as_ref() { + debug_assert!(got <= bytes, "recovery read over-reported byte count"); + if let Some(ev) = self.adaptive.on_success(sectors) { + self.emit(ev); + } + let got = got.min(bytes); + self.buf_valid = got; + self.current_offset += sectors as u32; + self.bytes_read_total = self.bytes_read_total.saturating_add(got as u64); + self.emit(EventKind::BytesRead { + bytes: self.bytes_read_total, + total: self.bytes_total_extents, + }); + break; + } + + // Recovery read also failed. Skip the WHOLE failed unit or bail. + // Zero-filling and advancing by the full unit keeps + // current_offset unit-aligned, so the next read still begins on a + // real AACS unit boundary (a 1-sector skip here would desync the + // rest of the title — the bug this guards). if self.skip_errors { let zb = sectors as usize * 2048; self.read_buf.resize(zb, 0); @@ -468,14 +504,9 @@ impl DiscStream { self.current_offset += sectors as u32; break; } else { - // Build the error from the failure we ALREADY hold. - // Re-reading the same known-bad LBA here doubled drive - // abuse (hard rule #2: repeated failed reads on the - // same LBA push the BU40N into fast-fail) and, if the - // retry transiently succeeded, dropped the good data - // and returned a bogus status=0/sense=None error for a - // readable sector. - let err = res.err(); + // Build the error from the recovery failure we now hold + // (falling back to the original 10s-read failure). + let err = rec.err().or(res.err()); let (status, sense) = err.as_ref().map(extract_scsi_context).unwrap_or((0, None)); return Err(crate::error::Error::DiscRead {