From 0a7f3fe030ef7a25b49c40e2b1d4e01ffc50ba63 Mon Sep 17 00:00:00 2001 From: ltstriker Date: Wed, 30 Sep 2026 11:45:39 +0800 Subject: [PATCH 1/4] block: tighten discard handling for hole punching - Advertise the qcow2 cluster size as discard alignment. qcow2 only frees whole clusters, so smaller discards were silently dropped. - Don't advertise DISCARD / WRITE_ZEROES on read-only disks. - Reject DISCARD / WRITE_ZEROES ranges that extend past the disk. - Report VIRTIO_BLK_S_UNSUPP for unknown requests and unsupported discard/zeroing, instead of IOERR. Discards may now punch holes into the image file; note that this relies on the single block worker serialising all requests. Co-Authored-By: Claude Opus 5.5 --- src/devices/src/virtio/block/device.rs | 10 ++-- src/devices/src/virtio/block/worker.rs | 71 ++++++++++++++++++-------- 2 files changed, 57 insertions(+), 24 deletions(-) diff --git a/src/devices/src/virtio/block/device.rs b/src/devices/src/virtio/block/device.rs index c943b10e03..69650dd355 100644 --- a/src/devices/src/virtio/block/device.rs +++ b/src/devices/src/virtio/block/device.rs @@ -253,7 +253,7 @@ impl Block { #[cfg(target_os = "macos")] let file_opts = file_opts.relaxed_sync(sync_mode == SyncMode::Relaxed); let file = ImagoFile::open_sync(file_opts)?; - let discard_alignment = file.discard_align(); + let mut discard_alignment = file.discard_align(); let disk_image = match disk_image_format { ImageType::Qcow2 => { @@ -263,6 +263,8 @@ impl Block { !is_disk_read_only, )?; qcow2.open_implicit_dependencies_sync()?; + // qcow2 only frees whole clusters; smaller discards are dropped. + discard_alignment = discard_alignment.max(qcow2.cluster_size()); SyncFormatAccess::new(qcow2)? } ImageType::Raw => { @@ -288,8 +290,6 @@ impl Block { let mut avail_features = (1u64 << VIRTIO_F_VERSION_1) | (1u64 << VIRTIO_BLK_F_SEG_MAX) - | (1u64 << VIRTIO_BLK_F_DISCARD) - | (1u64 << VIRTIO_BLK_F_WRITE_ZEROES) | (1u64 << VIRTIO_RING_F_EVENT_IDX); if sync_mode != SyncMode::None { @@ -298,7 +298,9 @@ impl Block { if is_disk_read_only { avail_features |= 1u64 << VIRTIO_BLK_F_RO; - }; + } else { + avail_features |= (1u64 << VIRTIO_BLK_F_DISCARD) | (1u64 << VIRTIO_BLK_F_WRITE_ZEROES); + } let config = VirtioBlkConfig { capacity: disk_properties.nsectors(), diff --git a/src/devices/src/virtio/block/worker.rs b/src/devices/src/virtio/block/worker.rs index ad91fc9180..9038b0baf8 100644 --- a/src/devices/src/virtio/block/worker.rs +++ b/src/devices/src/virtio/block/worker.rs @@ -20,12 +20,30 @@ pub enum RequestError { DiscardingToZero(io::Error), FlushingToDisk(io::Error), InvalidDataLength, + InvalidRange, ReadingFromDescriptor(io::Error), WritingToDescriptor(io::Error), WritingZeroes(io::Error), UnknownRequest, } +impl RequestError { + /// virtio-blk status reported to the guest for this error. + fn status(&self) -> u32 { + match self { + RequestError::UnknownRequest => VIRTIO_BLK_S_UNSUPP, + RequestError::Discarding(e) + | RequestError::DiscardingToZero(e) + | RequestError::WritingZeroes(e) + if e.kind() == io::ErrorKind::Unsupported => + { + VIRTIO_BLK_S_UNSUPP + } + _ => VIRTIO_BLK_S_IOERR, + } + } +} + /// The request header represents the mandatory fields of each block device request. /// /// A request header contains the following fields: @@ -187,7 +205,7 @@ impl BlockWorker { Ok(l) => (VIRTIO_BLK_S_OK.try_into().unwrap(), l), Err(e) => { error!("error processing request: {e:?}"); - (VIRTIO_BLK_S_IOERR.try_into().unwrap(), 0) + (e.status().try_into().unwrap(), 0) } }; @@ -259,45 +277,35 @@ impl BlockWorker { Ok(disk_id.len()) } } + // Discards may punch holes into the image file, which is only safe because this + // worker handles all requests of the disk one at a time (no I/O can be in flight + // on the discarded range). Revisit this before adding more queues or workers. VIRTIO_BLK_T_DISCARD => { - let discard_write_data: DiscardWriteData = reader - .read_obj() - .map_err(RequestError::ReadingFromDescriptor)?; + let (offset, length) = self.read_discard_range(reader)?.0; self.disk .file .lock() .unwrap() - .discard_to_any( - discard_write_data.sector * 512, - discard_write_data.num_sectors as u64 * 512, - ) + .discard_to_any(offset, length) .map_err(RequestError::Discarding)?; Ok(0) } VIRTIO_BLK_T_WRITE_ZEROES => { - let discard_write_data: DiscardWriteData = reader - .read_obj() - .map_err(RequestError::ReadingFromDescriptor)?; - let unmap = (discard_write_data.flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) != 0; + let ((offset, length), flags) = self.read_discard_range(reader)?; + let unmap = (flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) != 0; if unmap { self.disk .file .lock() .unwrap() - .discard_to_zero( - discard_write_data.sector * 512, - discard_write_data.num_sectors as u64 * 512, - ) + .discard_to_zero(offset, length) .map_err(RequestError::DiscardingToZero)?; } else { self.disk .file .lock() .unwrap() - .write_zeroes( - discard_write_data.sector * 512, - discard_write_data.num_sectors as u64 * 512, - ) + .write_zeroes(offset, length) .map_err(RequestError::WritingZeroes)?; } Ok(0) @@ -305,4 +313,27 @@ impl BlockWorker { _ => Err(RequestError::UnknownRequest), } } + + /// Read a DISCARD / WRITE_ZEROES segment, returning its byte range and flags. + /// + /// Fails if the range does not lie entirely within the disk. + fn read_discard_range( + &self, + reader: &mut Reader, + ) -> result::Result<((u64, u64), u32), RequestError> { + let data: DiscardWriteData = reader + .read_obj() + .map_err(RequestError::ReadingFromDescriptor)?; + let end_sector = data + .sector + .checked_add(data.num_sectors as u64) + .ok_or(RequestError::InvalidRange)?; + if end_sector > self.disk.nsectors() { + return Err(RequestError::InvalidRange); + } + Ok(( + (data.sector * 512, data.num_sectors as u64 * 512), + data.flags, + )) + } } From 719c4cb6df738313bd1e239be9f3c57a92ab3959 Mon Sep 17 00:00:00 2001 From: ltstriker Date: Wed, 30 Sep 2026 14:50:03 +0800 Subject: [PATCH 2/4] devices: use imago with qcow2 hole punching Switch imago to a 0.2.2-based fork that punches holes in the image file for freed data clusters, and persists refcounts before referencing new L1/L2 tables. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 5 ++--- src/devices/Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e2c7a049cb..f1d791dc93 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -478,9 +478,8 @@ checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" [[package]] name = "imago" -version = "0.2.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6e8e4b92aa0dd860579cfba776dbf0918a3a7ac5cb601af7d3fc835e71592a5b" +version = "0.2.2+boxlite.1" +source = "git+https://github.com/ltstriker/imago?rev=0ff6430e7ee05c9920dd84809388935a1a340396#0ff6430e7ee05c9920dd84809388935a1a340396" dependencies = [ "async-trait", "bincode", diff --git a/src/devices/Cargo.toml b/src/devices/Cargo.toml index 4f5125d9af..fecb2052f4 100644 --- a/src/devices/Cargo.toml +++ b/src/devices/Cargo.toml @@ -41,7 +41,7 @@ arch = { package = "krun-arch", version = "=0.1.0-1.18.0", path = "../arch" } utils = { package = "krun-utils", version = "=0.1.0-1.18.0", path = "../utils" } polly = { package = "krun-polly", version = "=0.1.0-1.18.0", path = "../polly" } rutabaga_gfx = { package = "krun-rutabaga-gfx", version = "=0.1.0-1.18.0", path = "../rutabaga_gfx", features = ["virgl_renderer", "virgl_renderer_next"], optional = true } -imago = { version = "0.2.2", features = ["sync-wrappers", "vm-memory"] } +imago = { git = "https://github.com/ltstriker/imago", rev = "0ff6430e7ee05c9920dd84809388935a1a340396", features = ["sync-wrappers", "vm-memory"] } [target.'cfg(target_os = "macos")'.dependencies] hvf = { package = "krun-hvf", version = "=0.1.0-1.18.0", path = "../hvf" } From 20cd22bd9a8f9591fd97259d992f57616388b4da Mon Sep 17 00:00:00 2001 From: ltstriker Date: Wed, 30 Sep 2026 15:23:44 +0800 Subject: [PATCH 3/4] block: keep discard changes to what hole punching needs Drop the read-only feature and VIRTIO_BLK_S_UNSUPP changes: neither fixes an observed problem. Move the DISCARD / WRITE_ZEROES range check into discard_byte_range() and test it, including a sector whose byte offset would wrap around to the start of the disk. Co-Authored-By: Claude Opus 5.5 --- src/devices/src/virtio/block/device.rs | 6 +- src/devices/src/virtio/block/worker.rs | 88 +++++++++++++++++--------- 2 files changed, 62 insertions(+), 32 deletions(-) diff --git a/src/devices/src/virtio/block/device.rs b/src/devices/src/virtio/block/device.rs index 69650dd355..2ea4ed74b2 100644 --- a/src/devices/src/virtio/block/device.rs +++ b/src/devices/src/virtio/block/device.rs @@ -290,6 +290,8 @@ impl Block { let mut avail_features = (1u64 << VIRTIO_F_VERSION_1) | (1u64 << VIRTIO_BLK_F_SEG_MAX) + | (1u64 << VIRTIO_BLK_F_DISCARD) + | (1u64 << VIRTIO_BLK_F_WRITE_ZEROES) | (1u64 << VIRTIO_RING_F_EVENT_IDX); if sync_mode != SyncMode::None { @@ -298,9 +300,7 @@ impl Block { if is_disk_read_only { avail_features |= 1u64 << VIRTIO_BLK_F_RO; - } else { - avail_features |= (1u64 << VIRTIO_BLK_F_DISCARD) | (1u64 << VIRTIO_BLK_F_WRITE_ZEROES); - } + }; let config = VirtioBlkConfig { capacity: disk_properties.nsectors(), diff --git a/src/devices/src/virtio/block/worker.rs b/src/devices/src/virtio/block/worker.rs index 9038b0baf8..fc5103ab9f 100644 --- a/src/devices/src/virtio/block/worker.rs +++ b/src/devices/src/virtio/block/worker.rs @@ -27,23 +27,6 @@ pub enum RequestError { UnknownRequest, } -impl RequestError { - /// virtio-blk status reported to the guest for this error. - fn status(&self) -> u32 { - match self { - RequestError::UnknownRequest => VIRTIO_BLK_S_UNSUPP, - RequestError::Discarding(e) - | RequestError::DiscardingToZero(e) - | RequestError::WritingZeroes(e) - if e.kind() == io::ErrorKind::Unsupported => - { - VIRTIO_BLK_S_UNSUPP - } - _ => VIRTIO_BLK_S_IOERR, - } - } -} - /// The request header represents the mandatory fields of each block device request. /// /// A request header contains the following fields: @@ -205,7 +188,7 @@ impl BlockWorker { Ok(l) => (VIRTIO_BLK_S_OK.try_into().unwrap(), l), Err(e) => { error!("error processing request: {e:?}"); - (e.status().try_into().unwrap(), 0) + (VIRTIO_BLK_S_IOERR.try_into().unwrap(), 0) } }; @@ -324,16 +307,63 @@ impl BlockWorker { let data: DiscardWriteData = reader .read_obj() .map_err(RequestError::ReadingFromDescriptor)?; - let end_sector = data - .sector - .checked_add(data.num_sectors as u64) - .ok_or(RequestError::InvalidRange)?; - if end_sector > self.disk.nsectors() { - return Err(RequestError::InvalidRange); - } - Ok(( - (data.sector * 512, data.num_sectors as u64 * 512), - data.flags, - )) + let range = discard_byte_range(data.sector, data.num_sectors, self.disk.nsectors())?; + Ok((range, data.flags)) + } +} + +/// Byte range of a DISCARD / WRITE_ZEROES segment on a disk of `disk_sectors` sectors. +/// +/// Both values come from the guest. Discards may punch holes into the image file, so a range +/// past the end of the disk, or one whose byte offset would overflow and wrap around to the +/// start of the disk, must be rejected rather than executed. +fn discard_byte_range( + sector: u64, + num_sectors: u32, + disk_sectors: u64, +) -> result::Result<(u64, u64), RequestError> { + match sector.checked_add(num_sectors as u64) { + Some(end) if end <= disk_sectors => Ok((sector * 512, num_sectors as u64 * 512)), + _ => Err(RequestError::InvalidRange), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const DISK_SECTORS: u64 = 2048; + + #[test] + fn discard_range_within_disk_is_converted_to_bytes() { + assert!(matches!( + discard_byte_range(8, 16, DISK_SECTORS), + Ok((4096, 8192)) + )); + assert!(matches!( + discard_byte_range(DISK_SECTORS - 16, 16, DISK_SECTORS), + Ok((_, 8192)) + )); + } + + #[test] + fn discard_range_past_disk_end_is_rejected() { + assert!(matches!( + discard_byte_range(DISK_SECTORS - 8, 16, DISK_SECTORS), + Err(RequestError::InvalidRange) + )); + } + + #[test] + fn discard_range_that_would_wrap_to_disk_start_is_rejected() { + // sector * 512 wraps around to byte offset 0 + assert!(matches!( + discard_byte_range(1 << 55, 1, DISK_SECTORS), + Err(RequestError::InvalidRange) + )); + assert!(matches!( + discard_byte_range(u64::MAX, 1, DISK_SECTORS), + Err(RequestError::InvalidRange) + )); } } From a0f61cfd5da2e6024f5cf32edfc3dca9a0c842ce Mon Sep 17 00:00:00 2001 From: ltstriker Date: Wed, 30 Sep 2026 15:26:19 +0800 Subject: [PATCH 4/4] block: keep the default discard alignment Advertising the qcow2 cluster size as discard alignment made no measurable difference to reclaimed space (same result with 512 B and 64 KiB granularity in the guest), so drop it. Pin imago to the commit that no longer carries the accessor it needed. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 2 +- src/devices/Cargo.toml | 2 +- src/devices/src/virtio/block/device.rs | 4 +--- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f1d791dc93..23c5f83f70 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -479,7 +479,7 @@ checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" [[package]] name = "imago" version = "0.2.2+boxlite.1" -source = "git+https://github.com/ltstriker/imago?rev=0ff6430e7ee05c9920dd84809388935a1a340396#0ff6430e7ee05c9920dd84809388935a1a340396" +source = "git+https://github.com/ltstriker/imago?rev=0923b1c9ff3102a9ac57a9615e729c09aa91a5c3#0923b1c9ff3102a9ac57a9615e729c09aa91a5c3" dependencies = [ "async-trait", "bincode", diff --git a/src/devices/Cargo.toml b/src/devices/Cargo.toml index fecb2052f4..fd619a7e79 100644 --- a/src/devices/Cargo.toml +++ b/src/devices/Cargo.toml @@ -41,7 +41,7 @@ arch = { package = "krun-arch", version = "=0.1.0-1.18.0", path = "../arch" } utils = { package = "krun-utils", version = "=0.1.0-1.18.0", path = "../utils" } polly = { package = "krun-polly", version = "=0.1.0-1.18.0", path = "../polly" } rutabaga_gfx = { package = "krun-rutabaga-gfx", version = "=0.1.0-1.18.0", path = "../rutabaga_gfx", features = ["virgl_renderer", "virgl_renderer_next"], optional = true } -imago = { git = "https://github.com/ltstriker/imago", rev = "0ff6430e7ee05c9920dd84809388935a1a340396", features = ["sync-wrappers", "vm-memory"] } +imago = { git = "https://github.com/ltstriker/imago", rev = "0923b1c9ff3102a9ac57a9615e729c09aa91a5c3", features = ["sync-wrappers", "vm-memory"] } [target.'cfg(target_os = "macos")'.dependencies] hvf = { package = "krun-hvf", version = "=0.1.0-1.18.0", path = "../hvf" } diff --git a/src/devices/src/virtio/block/device.rs b/src/devices/src/virtio/block/device.rs index 2ea4ed74b2..c943b10e03 100644 --- a/src/devices/src/virtio/block/device.rs +++ b/src/devices/src/virtio/block/device.rs @@ -253,7 +253,7 @@ impl Block { #[cfg(target_os = "macos")] let file_opts = file_opts.relaxed_sync(sync_mode == SyncMode::Relaxed); let file = ImagoFile::open_sync(file_opts)?; - let mut discard_alignment = file.discard_align(); + let discard_alignment = file.discard_align(); let disk_image = match disk_image_format { ImageType::Qcow2 => { @@ -263,8 +263,6 @@ impl Block { !is_disk_read_only, )?; qcow2.open_implicit_dependencies_sync()?; - // qcow2 only frees whole clusters; smaller discards are dropped. - discard_alignment = discard_alignment.max(qcow2.cluster_size()); SyncFormatAccess::new(qcow2)? } ImageType::Raw => {