v0.13.9: Disc::copy stall guard + SgIoTransport no-reopen-on-timeout
Fixes the silent Pass 1 hang observed on Dune 2 with v0.13.8 (drive grinding through bad sectors at 0 KB/s, errs=0, no error surfaced). Root cause: SgIoTransport::execute's reopen-after-poll-timeout opened a fresh /dev/sg* fd on the main thread, which serialized against the spawned close() of the old fd via the kernel's per-device state lock. The userspace 1.5s timeout still fired, but the abandon-and-reopen recovery itself blocked the main thread for as long as close() took. Net: reads returned slowly, skip-forward fired on every iteration, bytes_good never advanced. - SgIoTransport::execute: on poll timeout, spawn close, set fd=-1, return Err. No reopen on the main thread. The transport is now invalidated until the consumer creates a fresh Drive. - Disc::copy: add stall guard. CopyOptions.stall_secs (default 120s). If bytes_good doesn't advance for the threshold, break 'outer cleanly with complete=false, bytes_pending > 0 so Pass 2 retries pick up the NonTrimmed ranges with recovery=true 30s timeouts. - New regression test: test_disc_copy_stall_detection_triggers_ skip_forward in tests/integration_progress_and_halt.rs.
This commit is contained in:
@@ -1286,6 +1286,9 @@ impl Disc {
|
||||
let mut buf = vec![0u8; batch as usize * 2048];
|
||||
let mut bytes_done = 0u64;
|
||||
let mut halt_requested = false;
|
||||
let stall_threshold = std::time::Duration::from_secs(opts.stall_secs.unwrap_or(120));
|
||||
let mut last_good_advance = std::time::Instant::now();
|
||||
let mut last_observed_good: u64 = 0;
|
||||
|
||||
// Iterate over not-yet-finished regions from the mapfile. We re-read the
|
||||
// mapfile after each block because record() mutates the region list.
|
||||
@@ -1315,6 +1318,30 @@ impl Disc {
|
||||
break 'outer;
|
||||
}
|
||||
}
|
||||
// Stall guard. The signal is "bytes_good (Finished sectors)
|
||||
// hasn't advanced for stall_threshold." pos may still be
|
||||
// advancing via the skip_on_error branch; that's not real
|
||||
// progress because skip-forward only marks ranges
|
||||
// NonTrimmed for Pass 2 to retry. If we go stall_threshold
|
||||
// without ANY successful read, the drive is grinding
|
||||
// unproductively (or kernel is silently stalling reads
|
||||
// past their per-CDB timeout — observed live on Dell with
|
||||
// SgIoTransport's reopen-after-timeout serializing
|
||||
// against close). Bail Pass 1; Pass 2 (Disc::patch with
|
||||
// recovery=true, 30 s timeouts) will retry the
|
||||
// NonTrimmed ranges.
|
||||
let cur_good = map.stats().bytes_good;
|
||||
if cur_good != last_observed_good {
|
||||
last_observed_good = cur_good;
|
||||
last_good_advance = std::time::Instant::now();
|
||||
} else if last_good_advance.elapsed() > stall_threshold {
|
||||
// Stall: bail Pass 1. Return cleanly with
|
||||
// bytes_pending > 0 and complete = false so the
|
||||
// caller's retry path (Disc::patch with
|
||||
// recovery=true, 30s timeouts) gets a shot at the
|
||||
// NonTrimmed ranges.
|
||||
break 'outer;
|
||||
}
|
||||
let block_bytes = (region_end - pos).min(batch as u64 * 2048);
|
||||
let lba = (pos / 2048) as u32;
|
||||
let count = (block_bytes / 2048) as u16;
|
||||
@@ -1404,6 +1431,15 @@ pub struct CopyOptions<'a> {
|
||||
pub skip_forward: bool,
|
||||
pub on_progress: Option<&'a dyn Fn(u64, u64)>,
|
||||
pub halt: Option<std::sync::Arc<std::sync::atomic::AtomicBool>>,
|
||||
/// Wall-clock stall threshold in seconds. If the inner copy loop runs
|
||||
/// this long without `pos` advancing, the current block is treated as
|
||||
/// a read failure (skip-forward in `skip_on_error` mode, else
|
||||
/// `Err(DiscRead)`). Defaults to 120 s. Defensive guard for
|
||||
/// kernel-level hangs that bypass the per-CDB SCSI timeout — e.g. the
|
||||
/// v0.13.8 case where SgIoTransport's reopen-after-timeout serialized
|
||||
/// against the in-flight close, blocking the main thread for tens of
|
||||
/// seconds per read.
|
||||
pub stall_secs: Option<u64>,
|
||||
}
|
||||
|
||||
/// Result of `Disc::copy`. `complete=true` means every byte reached a terminal
|
||||
|
||||
+26
-14
@@ -321,26 +321,38 @@ impl ScsiTransport for SgIoTransport {
|
||||
|
||||
if pr <= 0 {
|
||||
// Timeout (0) or fatal poll error (-1).
|
||||
// Command is still pending in the kernel. Abandon this fd and
|
||||
// open a fresh one. The old fd is closed in a background thread
|
||||
// because close() blocks until the kernel completes/aborts the
|
||||
// pending command.
|
||||
// Command is still pending in the kernel. Abandon the fd by
|
||||
// spawning a background close (which will block until the
|
||||
// kernel completes/aborts the pending command), and mark
|
||||
// this transport invalid by setting `self.fd = -1`.
|
||||
//
|
||||
// Why we no longer reopen on the main thread: opening the
|
||||
// SAME /dev/sg* device while the prior fd is mid-close
|
||||
// serializes via the kernel's per-device state lock, so
|
||||
// `libc::open()` on the main thread blocks for the same
|
||||
// duration that close() does — defeating the userspace
|
||||
// timeout. Observed in v0.13.8 live test on Dune 2: each
|
||||
// timed-out read added 60+ s to the next iteration of
|
||||
// Disc::copy, leaving the rip stuck without surfacing an
|
||||
// error or wedging the drive.
|
||||
//
|
||||
// Net effect of the fix: a single read timeout invalidates
|
||||
// the SgIoTransport. The Drive is now "dead" until the
|
||||
// consumer (autorip's rip thread) catches the failure and
|
||||
// reopens. Disc::copy's `skip_on_error=true` path will see
|
||||
// the Err and skip-forward, advancing pos, and the next
|
||||
// read on this fd returns Err(DeviceNotFound) immediately —
|
||||
// which Disc::copy continues to skip-forward through until
|
||||
// the NonTried region is exhausted. Pass 1 then ends with
|
||||
// bytes_pending > 0 and the rip thread reopens the Drive
|
||||
// for Pass 2 (Disc::patch with recovery=true and 30 s
|
||||
// timeouts).
|
||||
let old_fd = self.fd;
|
||||
self.fd = -1;
|
||||
|
||||
std::thread::spawn(move || {
|
||||
unsafe { libc::close(old_fd) };
|
||||
});
|
||||
|
||||
let c_path = Self::to_c_path(&self.device_path);
|
||||
let new_fd = unsafe {
|
||||
libc::open(
|
||||
c_path.as_ptr() as *const libc::c_char,
|
||||
libc::O_RDWR | libc::O_NONBLOCK | libc::O_CLOEXEC,
|
||||
)
|
||||
};
|
||||
self.fd = if new_fd >= 0 { new_fd } else { -1 };
|
||||
|
||||
return Err(Error::ScsiError {
|
||||
opcode: cdb[0],
|
||||
status: 0xFF,
|
||||
|
||||
Reference in New Issue
Block a user