v0.13.11: revert SgIoTransport timeout path — keep transport alive
v0.13.10's 'fd=-1 on first poll timeout' was too aggressive: a single transient killed the entire transport, Pass 1 finished in 45ms with 0 GB good on Dune 2. Revert to spawn-close + main-thread-reopen (the v0.13.5/8 pattern). Per-timeout cost is up to ~60s while the kernel completes the abandoned command, but the v0.13.9 Disc::copy stall guard caps catastrophic stalls at 120s of bytes_good non-advance. Pass 1 bails cleanly with NonTrimmed ranges; Pass 2 has a working Drive for retries with recovery=true + 30s timeouts.
This commit is contained in:
@@ -1,5 +1,37 @@
|
|||||||
# Changelog
|
# Changelog
|
||||||
|
|
||||||
|
## 0.13.11 (2026-04-25)
|
||||||
|
|
||||||
|
### Fix: revert SgIoTransport timeout path to keep transport alive
|
||||||
|
|
||||||
|
v0.13.10 changed `SgIoTransport::execute` to set `fd = -1` on a poll
|
||||||
|
timeout (no reopen on the main thread, since that would serialize
|
||||||
|
against the spawned close()). The intent was to escape the 60-s
|
||||||
|
blocking reopen.
|
||||||
|
|
||||||
|
The cost was too high: a single transient poll timeout permanently
|
||||||
|
killed the transport. Live test on Dune 2 (post-replug):
|
||||||
|
- Pass 1 ran for **45 ms** then returned with 0 GB good and 80 GB
|
||||||
|
pending.
|
||||||
|
- The first SCSI READ timed out, fd went to -1, every subsequent
|
||||||
|
read returned `DeviceNotFound` instantly, Disc::copy raced through
|
||||||
|
the entire disc skip-forwarding in milliseconds.
|
||||||
|
- Pass 2 inherited the dead Drive and was equally useless.
|
||||||
|
|
||||||
|
Revert: spawn close + reopen on main thread (the v0.13.5/8
|
||||||
|
behavior). Yes the main-thread open() may block up to ~60 s while
|
||||||
|
the kernel completes the abandoned command — but the v0.13.9
|
||||||
|
`Disc::copy` stall guard already caps catastrophic stalls at 120 s
|
||||||
|
of `bytes_good` non-advance. Net: per-timeout cost is ~60 s, but
|
||||||
|
Pass 1 cleanly bails out within 120 s of any wedge, and Pass 2 has
|
||||||
|
a working Drive to retry NonTrimmed ranges with `recovery=true` +
|
||||||
|
30 s timeouts.
|
||||||
|
|
||||||
|
The integration test for the stall guard
|
||||||
|
(`test_disc_copy_stall_detection_triggers_skip_forward`) continues
|
||||||
|
to pass — the guard fires regardless of which transport-recovery
|
||||||
|
strategy is in play.
|
||||||
|
|
||||||
## 0.13.10 (2026-04-25)
|
## 0.13.10 (2026-04-25)
|
||||||
|
|
||||||
### Version sync — no functional changes
|
### Version sync — no functional changes
|
||||||
|
|||||||
+1
-1
@@ -1,6 +1,6 @@
|
|||||||
[package]
|
[package]
|
||||||
name = "libfreemkv"
|
name = "libfreemkv"
|
||||||
version = "0.13.10"
|
version = "0.13.11"
|
||||||
edition = "2024"
|
edition = "2024"
|
||||||
rust-version = "1.86"
|
rust-version = "1.86"
|
||||||
license = "AGPL-3.0-only"
|
license = "AGPL-3.0-only"
|
||||||
|
|||||||
+30
-25
@@ -321,38 +321,43 @@ impl ScsiTransport for SgIoTransport {
|
|||||||
|
|
||||||
if pr <= 0 {
|
if pr <= 0 {
|
||||||
// Timeout (0) or fatal poll error (-1).
|
// Timeout (0) or fatal poll error (-1).
|
||||||
// Command is still pending in the kernel. Abandon the fd by
|
// Command is still pending in the kernel. Spawn a background
|
||||||
// spawning a background close (which will block until the
|
// close of the old fd (which blocks until the kernel
|
||||||
// kernel completes/aborts the pending command), and mark
|
// completes/aborts the pending command) and open a fresh fd
|
||||||
// this transport invalid by setting `self.fd = -1`.
|
// on the main thread. The main-thread open() can serialize
|
||||||
|
// against the in-flight close via the kernel's per-device
|
||||||
|
// state lock — so this call may block up to ~60 s while
|
||||||
|
// the kernel finishes the abandoned command. That's the
|
||||||
|
// cost of keeping the Drive alive across a timeout. The
|
||||||
|
// Disc::copy stall guard (v0.13.9, default 120 s of
|
||||||
|
// bytes_good non-advance) is the upper bound that prevents
|
||||||
|
// a catastrophic grind on a wedged read region.
|
||||||
//
|
//
|
||||||
// Why we no longer reopen on the main thread: opening the
|
// History:
|
||||||
// SAME /dev/sg* device while the prior fd is mid-close
|
// - 0.13.5 and earlier: same as this — but with no upper
|
||||||
// serializes via the kernel's per-device state lock, so
|
// bound, hence 45-min hangs.
|
||||||
// `libc::open()` on the main thread blocks for the same
|
// - 0.13.10: tried "set fd=-1, no reopen" — too aggressive,
|
||||||
// duration that close() does — defeating the userspace
|
// one transient timeout killed the whole transport, Pass
|
||||||
// timeout. Observed in v0.13.8 live test on Dune 2: each
|
// 1 finished in 45 ms with everything NonTrimmed.
|
||||||
// timed-out read added 60+ s to the next iteration of
|
// - 0.13.11 (this): same close+reopen as 0.13.5/8 BUT with
|
||||||
// Disc::copy, leaving the rip stuck without surfacing an
|
// the v0.13.9 stall guard ensuring Disc::copy bails out
|
||||||
// error or wedging the drive.
|
// cleanly within 120 s of zero forward progress.
|
||||||
//
|
|
||||||
// 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;
|
let old_fd = self.fd;
|
||||||
self.fd = -1;
|
self.fd = -1;
|
||||||
|
|
||||||
std::thread::spawn(move || {
|
std::thread::spawn(move || {
|
||||||
unsafe { libc::close(old_fd) };
|
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 {
|
return Err(Error::ScsiError {
|
||||||
opcode: cdb[0],
|
opcode: cdb[0],
|
||||||
status: 0xFF,
|
status: 0xFF,
|
||||||
|
|||||||
Reference in New Issue
Block a user