v0.13.23 — stop discarding the drive's SCSI sense data
Through the entire 0.13.x line, every CHECK CONDITION reply from the
drive (the standard way SCSI tells you why a sector failed) was being
collapsed into a synthetic status=0xFF, sense_key=0 transport-wedge
sentinel and the actual sense data was thrown away. Confirmed live on
the BU40N reading Dune 2 on 2026-04-27: drive returned host_status=0,
driver_status=8, status=2, exec_elapsed_ms=1416 on every bad sector
— a clean CHECK CONDITION carrying full sense data — and Disc::copy
was bailing on it as if the bridge had wedged.
Root cause: scsi/linux.rs's wedge check was
`host_status != 0 || driver_status != 0`
SG's DRIVER_SENSE bit (0x08) is set on every CHECK CONDITION reply
just to flag "sense buffer is populated" — it's not a transport
failure on its own. Pre-fix we conflated the two and silently lost
every drive-reported error reason. macOS and Windows backends had
the same shape: they extracted sense_key only, dropping ASC/ASCQ.
API restructure (clean separation):
Error::ScsiError {
opcode: u8,
status: u8, // 0xFF = synthetic transport-failure
sense: Option<ScsiSense>, // None ⇔ no sense delivered
}
pub struct ScsiSense { sense_key: u8, asc: u8, ascq: u8 }
impl ScsiSense {
pub fn is_marginal(&self) -> bool // keys 0/1/3/B
pub fn is_medium_error(&self) -> bool
pub fn is_hardware_error(&self) -> bool
pub fn is_unit_attention(&self) -> bool
pub fn is_data_protect(&self) -> bool
pub fn is_not_ready(&self) -> bool
pub fn is_illegal_request(&self) -> bool
pub fn is_aborted_command(&self) -> bool
}
impl Error {
pub fn scsi_sense(&self) -> Option<&ScsiSense>
pub fn is_scsi_transport_failure(&self) -> bool
pub fn is_marginal_read(&self) -> bool
}
SCSI protocol constants (SCSI_STATUS_*, SENSE_KEY_*) moved from
error.rs to scsi/mod.rs where they belong alongside SCSI_INQUIRY,
SCSI_READ_10, etc. parse_sense replaces parse_sense_key (returns the
full triple, not just the key); inline tests now exercise ASC/ASCQ
extraction at the right offsets for both descriptor (0x72/0x73) and
fixed (0x70/0x71) sense formats.
Disc::copy + Disc::patch sense-aware dispatch:
- marginal sense (MEDIUM ERROR / ABORTED COMMAND / RECOVERED ERROR
/ NO SENSE) → engage hysteresis (Block→Single, bpt=1)
- non-marginal sense (HARDWARE / DATA PROTECT / UNIT ATTENTION /
NOT READY / ILLEGAL REQUEST / transport failure / kernel
IoError) → bail with full sense info preserved; caller (autorip)
surfaces "physical replug" / "drive failing" / "media changed"
Pre-fix: every CHECK CONDITION → 0xFF synthetic → Disc::copy bailed
→ bytes_good froze at the bad zone. The hysteresis from v0.13.22
was correct but never got to run. This release unblocks it.
Disc::patch's wedged_threshold (50 consecutive failures) stays as
defense-in-depth for chains of marginal failures; a single
non-marginal sense now short-circuits it.
New phase=bail trace event records the bail reason with the sense
triple. phase=transport_err remains for genuine bridge wedges /
kernel timeouts; phase=scsi_err carries the parsed sense_key, asc,
ascq for drive-reported errors.
All 350 tests pass. Clippy clean across all targets.
This commit is contained in:
MattJackson
+80
-15
@@ -1382,16 +1382,14 @@ impl Disc {
|
||||
let read_t0 = block_t0;
|
||||
let block_bytes_usz = block_bytes as usize;
|
||||
iter_count += 1;
|
||||
let block_ok = reader
|
||||
.read_sectors(
|
||||
block_lba,
|
||||
block_count,
|
||||
&mut buf[..block_bytes_usz],
|
||||
recovery,
|
||||
)
|
||||
.is_ok();
|
||||
let block_result = reader.read_sectors(
|
||||
block_lba,
|
||||
block_count,
|
||||
&mut buf[..block_bytes_usz],
|
||||
recovery,
|
||||
);
|
||||
|
||||
if block_ok {
|
||||
if block_result.is_ok() {
|
||||
// Fast path — full block read cleanly.
|
||||
read_ok_count += 1;
|
||||
if opts.decrypt {
|
||||
@@ -1413,6 +1411,31 @@ impl Disc {
|
||||
return Err(Error::DiscRead {
|
||||
sector: block_lba as u64,
|
||||
});
|
||||
} else if !block_result
|
||||
.as_ref()
|
||||
.err()
|
||||
.map(Error::is_marginal_read)
|
||||
.unwrap_or(false)
|
||||
{
|
||||
// 0.13.23: SCSI sense-aware dispatch. Block read failed
|
||||
// with a sense class outside the marginal-read set
|
||||
// (real transport failure, HARDWARE ERROR, DATA
|
||||
// PROTECT, UNIT ATTENTION, NOT READY, ILLEGAL
|
||||
// REQUEST, kernel IoError). Bail with the full sense
|
||||
// triple preserved — caller (autorip) surfaces
|
||||
// "physical replug needed" / "drive failing" /
|
||||
// "media changed" / etc to the user. Hysteresis
|
||||
// would just hammer the same failure for thousands
|
||||
// of sectors at 1.4 sec each.
|
||||
let err = block_result.err().unwrap();
|
||||
tracing::trace!(
|
||||
target: "freemkv::disc",
|
||||
phase = "bail",
|
||||
lba = block_lba,
|
||||
error = %err,
|
||||
"block read failed with non-recoverable sense; bailing"
|
||||
);
|
||||
return Err(err);
|
||||
} else {
|
||||
// Block failed → drop to Single and read this range
|
||||
// sector-by-sector. Stay in Single across subsequent
|
||||
@@ -1445,9 +1468,30 @@ impl Disc {
|
||||
let s_lba = block_lba + s as u32;
|
||||
let s_pos = pos + (s as u64) * 2048;
|
||||
let one_bytes = 2048usize;
|
||||
let one_ok = reader
|
||||
.read_sectors(s_lba, 1, &mut buf[..one_bytes], recovery)
|
||||
.is_ok();
|
||||
let one_result =
|
||||
reader.read_sectors(s_lba, 1, &mut buf[..one_bytes], recovery);
|
||||
// 0.13.23: same sense-aware dispatch inside Single
|
||||
// mode. If a single-sector read fails with a
|
||||
// non-marginal sense (transport / hardware /
|
||||
// DATA PROTECT / UNIT ATTENTION / NOT READY /
|
||||
// ILLEGAL REQUEST / kernel IoError), the drive
|
||||
// isn't going to start succeeding for the next
|
||||
// 60 sectors either — bail with full sense info
|
||||
// rather than chewing through bpt=1 timeouts.
|
||||
if let Err(ref e) = one_result {
|
||||
if !e.is_marginal_read() {
|
||||
let err = one_result.err().unwrap();
|
||||
tracing::trace!(
|
||||
target: "freemkv::disc",
|
||||
phase = "bail",
|
||||
lba = s_lba,
|
||||
error = %err,
|
||||
"bpt=1 read failed with non-marginal sense; bailing"
|
||||
);
|
||||
return Err(err);
|
||||
}
|
||||
}
|
||||
let one_ok = one_result.is_ok();
|
||||
if one_ok {
|
||||
read_ok_count += 1;
|
||||
consecutive_good = consecutive_good.saturating_add(1);
|
||||
@@ -1769,9 +1813,30 @@ impl Disc {
|
||||
let count = (block_bytes / 2048) as u16;
|
||||
let bytes = count as usize * 2048;
|
||||
blocks_attempted += 1;
|
||||
let read_ok = reader
|
||||
.read_sectors(lba, count, &mut buf[..bytes], recovery)
|
||||
.is_ok();
|
||||
let read_result = reader.read_sectors(lba, count, &mut buf[..bytes], recovery);
|
||||
// 0.13.23: parity with Disc::copy — bail immediately on a
|
||||
// non-marginal SCSI sense (transport failure, HARDWARE
|
||||
// ERROR, DATA PROTECT, UNIT ATTENTION, NOT READY,
|
||||
// ILLEGAL REQUEST, kernel IoError). The wedged_threshold
|
||||
// counter is a defense-in-depth backstop for ~50
|
||||
// consecutive marginal failures, but a single
|
||||
// non-marginal error already proves the drive can't
|
||||
// produce data this pass — don't waste 50× the timeout
|
||||
// budget proving it again.
|
||||
if let Err(ref e) = read_result {
|
||||
if !e.is_marginal_read() {
|
||||
let err = read_result.err().unwrap();
|
||||
tracing::trace!(
|
||||
target: "freemkv::disc",
|
||||
phase = "patch_bail",
|
||||
lba,
|
||||
error = %err,
|
||||
"patch read failed with non-marginal sense; bailing"
|
||||
);
|
||||
return Err(err);
|
||||
}
|
||||
}
|
||||
let read_ok = read_result.is_ok();
|
||||
if read_ok {
|
||||
blocks_read_ok += 1;
|
||||
consecutive_failures = 0;
|
||||
|
||||
Reference in New Issue
Block a user