From 670397c1098ba8b40bed62c195dc3d1af8215e02 Mon Sep 17 00:00:00 2001 From: Sergio Lopez Date: Thu, 27 Nov 2025 15:54:37 +0100 Subject: [PATCH 1/5] devices: bump imago crate to 0.1.6 We need the latest version of the image crate to support F_DISCARD and F_WRITE_ZEROES. Signed-off-by: Sergio Lopez --- Cargo.lock | 4 ++-- src/devices/Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c3547d469..30dfe7dff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -739,9 +739,9 @@ dependencies = [ [[package]] name = "imago" -version = "0.1.5" +version = "0.1.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6a94a5babb4a05f462d7736c7263ddd7e721ec0c5e5f26b02d888e9edf75b549" +checksum = "a8e45dce78f8a4700ea570488b35af0d192ee0dd355a1d625f07f6ea49ac25bd" dependencies = [ "async-trait", "bincode", diff --git a/src/devices/Cargo.toml b/src/devices/Cargo.toml index b5024d75a..298c4dcdf 100644 --- a/src/devices/Cargo.toml +++ b/src/devices/Cargo.toml @@ -38,7 +38,7 @@ arch = { path = "../arch" } utils = { path = "../utils" } polly = { path = "../polly" } rutabaga_gfx = { path = "../rutabaga_gfx", features = ["virgl_renderer", "virgl_renderer_next"], optional = true } -imago = { version = "0.1.5", features = ["sync-wrappers", "vm-memory"] } +imago = { version = "0.1.6", features = ["sync-wrappers", "vm-memory"] } [target.'cfg(target_os = "macos")'.dependencies] hvf = { path = "../hvf" } From a075af9231287f9c0ef9d3648b54bfbd4f42b1d3 Mon Sep 17 00:00:00 2001 From: Sergio Lopez Date: Wed, 12 Nov 2025 12:23:15 +0100 Subject: [PATCH 2/5] virtio/block: extend VirtioBlkConfig Extend VirtioBlkConfig enough to cover up to write_zeroes_may_unmap, as we're going to need those fields for implementing support both F_DISCARD and F_WRITE_ZEROES. Signed-off-by: Sergio Lopez --- src/devices/src/virtio/block/device.rs | 30 ++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/devices/src/virtio/block/device.rs b/src/devices/src/virtio/block/device.rs index c4999accc..7ce704fa9 100644 --- a/src/devices/src/virtio/block/device.rs +++ b/src/devices/src/virtio/block/device.rs @@ -161,12 +161,41 @@ impl Drop for DiskProperties { } } +#[derive(Copy, Clone, Debug, Default)] +#[repr(C, packed)] +struct VirtioBlkGeometry { + cylinders: u16, + heads: u8, + sectors: u8, +} + +#[derive(Copy, Clone, Debug, Default)] +#[repr(C, packed)] +struct VirtioBlkTopology { + physical_block_exp: u8, + alignment_offset: u8, + min_io_size: u16, + opt_io_size: u32, +} + #[derive(Copy, Clone, Debug, Default)] #[repr(C, packed)] struct VirtioBlkConfig { capacity: u64, size_max: u32, seg_max: u32, + geometry: VirtioBlkGeometry, + blk_size: u32, + topology: VirtioBlkTopology, + writeback: u8, + unused0: u8, + num_queues: u16, + max_discard_sectors: u32, + max_discard_seg: u32, + discard_sector_alignment: u32, + max_write_zeroes_sectors: u32, + max_write_zeroes_seg: u32, + write_zeroes_may_unmap: u8, } // Safe because it only has data and has no implicit padding. @@ -265,6 +294,7 @@ impl Block { size_max: 0, // QUEUE_SIZE - 2 seg_max: 254, + ..Default::default() }; Ok(Block { From 8e6e5ca70e5e92dca1ad78691b1cf776f81c5ebf Mon Sep 17 00:00:00 2001 From: Sergio Lopez Date: Wed, 12 Nov 2025 17:59:31 +0100 Subject: [PATCH 3/5] virtio/block: wrap disk_image in a Mutex Imago's discard_to_* operations require a mutable reference to Storage. Let's wrap disk_image in a Mutex so we can obtain such reference when needed. Since, in practice, we won't have multiple threads accessing the lock, the overhead should be very small (one or two operations on atomics). Signed-off-by: Sergio Lopez --- src/devices/src/virtio/block/device.rs | 22 +++++++++------------- src/devices/src/virtio/block/worker.rs | 2 +- src/devices/src/virtio/file_traits.rs | 4 ++-- 3 files changed, 12 insertions(+), 16 deletions(-) diff --git a/src/devices/src/virtio/block/device.rs b/src/devices/src/virtio/block/device.rs index 7ce704fa9..e415842d6 100644 --- a/src/devices/src/virtio/block/device.rs +++ b/src/devices/src/virtio/block/device.rs @@ -15,7 +15,7 @@ use std::os::linux::fs::MetadataExt; use std::os::macos::fs::MetadataExt; use std::path::PathBuf; use std::result; -use std::sync::Arc; +use std::sync::{Arc, Mutex}; use std::thread::JoinHandle; use imago::{ @@ -65,18 +65,18 @@ impl CacheType { /// Helper object for setting up all `Block` fields derived from its backing file. pub(crate) struct DiskProperties { cache_type: CacheType, - pub(crate) file: Arc>>, + pub(crate) file: Arc>>>, nsectors: u64, image_id: Vec, } impl DiskProperties { pub fn new( - disk_image: Arc>>, + disk_image: Arc>>>, disk_image_id: Vec, cache_type: CacheType, ) -> io::Result { - let disk_size = disk_image.size(); + let disk_size = disk_image.lock().unwrap().size(); // We only support disk size, which uses the first two words of the configuration space. // If the image is not a multiple of the sector size, the tail bits are not exposed. @@ -95,10 +95,6 @@ impl DiskProperties { }) } - pub fn file(&self) -> &SyncFormatAccess> { - self.file.as_ref() - } - pub fn nsectors(&self) -> u64 { self.nsectors } @@ -146,11 +142,11 @@ impl Drop for DiskProperties { match self.cache_type { CacheType::Writeback => { // flush() first to force any cached data out. - if self.file.flush().is_err() { + if self.file.lock().unwrap().flush().is_err() { error!("Failed to flush block data on drop."); } // Sync data out to physical media on host. - if self.file.sync().is_err() { + if self.file.lock().unwrap().sync().is_err() { error!("Failed to sync block data on drop.") } } @@ -206,7 +202,7 @@ pub struct Block { // Host file and properties. disk: Option, cache_type: CacheType, - disk_image: Arc>>, + disk_image: Arc>>>, disk_image_id: Vec, worker_thread: Option>, worker_stopfd: EventFd, @@ -271,10 +267,10 @@ impl Block { } }; - let disk_image = Arc::new(disk_image); + let disk_image = Arc::new(Mutex::new(disk_image)); let disk_properties = - DiskProperties::new(Arc::clone(&disk_image), disk_image_id.clone(), cache_type)?; + DiskProperties::new(disk_image.clone(), disk_image_id.clone(), cache_type)?; let mut avail_features = (1u64 << VIRTIO_F_VERSION_1) | (1u64 << VIRTIO_BLK_F_FLUSH) diff --git a/src/devices/src/virtio/block/worker.rs b/src/devices/src/virtio/block/worker.rs index 2175b3bf6..a401d0bb5 100644 --- a/src/devices/src/virtio/block/worker.rs +++ b/src/devices/src/virtio/block/worker.rs @@ -228,7 +228,7 @@ impl BlockWorker { } VIRTIO_BLK_T_FLUSH => match self.disk.cache_type() { CacheType::Writeback => { - let diskfile = self.disk.file(); + let diskfile = self.disk.file.lock().unwrap(); diskfile.flush().map_err(RequestError::FlushingToDisk)?; diskfile.sync().map_err(RequestError::FlushingToDisk)?; Ok(0) diff --git a/src/devices/src/virtio/file_traits.rs b/src/devices/src/virtio/file_traits.rs index 72ab0e315..e7d2ca17d 100644 --- a/src/devices/src/virtio/file_traits.rs +++ b/src/devices/src/virtio/file_traits.rs @@ -432,7 +432,7 @@ impl FileReadWriteAtVolatile for DiskProperties { .len() .try_into() .map_err(|e| Error::new(ErrorKind::InvalidData, e))?; - self.file().readv(iovec, offset)?; + self.file.lock().unwrap().readv(iovec, offset)?; Ok(full_length) } @@ -450,7 +450,7 @@ impl FileReadWriteAtVolatile for DiskProperties { .len() .try_into() .map_err(|e| Error::new(ErrorKind::InvalidData, e))?; - self.file().writev(iovec, offset)?; + self.file.lock().unwrap().writev(iovec, offset)?; Ok(full_length) } } From 6a3686adbfdd2f6e2657976f1cab54e2d99bf578 Mon Sep 17 00:00:00 2001 From: Sergio Lopez Date: Wed, 12 Nov 2025 12:02:39 +0100 Subject: [PATCH 4/5] virtio/block: implement support for F_DISCARD The F_DISCARD feature enables the guest to request us to free up sectors on the backing storage. Since imago does most of the heavy lifting, we just need to announce the feature and call to imago's "discard_to_any" then needed. Signed-off-by: Sergio Lopez --- src/devices/src/virtio/block/device.rs | 5 +++++ src/devices/src/virtio/block/worker.rs | 27 +++++++++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/devices/src/virtio/block/device.rs b/src/devices/src/virtio/block/device.rs index e415842d6..13d8992bb 100644 --- a/src/devices/src/virtio/block/device.rs +++ b/src/devices/src/virtio/block/device.rs @@ -247,6 +247,7 @@ impl Block { #[cfg(target_os = "macos")] let file_opts = file_opts.relaxed_sync(true); let file = ImagoFile::open_sync(file_opts)?; + let discard_alignment = file.discard_align(); let disk_image = match disk_image_format { ImageType::Qcow2 => { @@ -275,6 +276,7 @@ impl Block { let mut avail_features = (1u64 << VIRTIO_F_VERSION_1) | (1u64 << VIRTIO_BLK_F_FLUSH) | (1u64 << VIRTIO_BLK_F_SEG_MAX) + | (1u64 << VIRTIO_BLK_F_DISCARD) | (1u64 << VIRTIO_RING_F_EVENT_IDX); if is_disk_read_only { @@ -290,6 +292,9 @@ impl Block { size_max: 0, // QUEUE_SIZE - 2 seg_max: 254, + max_discard_sectors: u32::MAX, + max_discard_seg: 1, + discard_sector_alignment: discard_alignment as u32 / 512, ..Default::default() }; diff --git a/src/devices/src/virtio/block/worker.rs b/src/devices/src/virtio/block/worker.rs index a401d0bb5..890e8995c 100644 --- a/src/devices/src/virtio/block/worker.rs +++ b/src/devices/src/virtio/block/worker.rs @@ -16,6 +16,7 @@ use vm_memory::{ByteValued, GuestMemoryMmap}; #[allow(dead_code)] #[derive(Debug)] pub enum RequestError { + Discarding(io::Error), FlushingToDisk(io::Error), InvalidDataLength, ReadingFromDescriptor(io::Error), @@ -39,10 +40,19 @@ pub struct RequestHeader { _reserved: u32, sector: u64, } - // Safe because RequestHeader only contains plain data. unsafe impl ByteValued for RequestHeader {} +#[derive(Copy, Clone, Default)] +#[repr(C)] +pub struct DiscardWriteData { + sector: u64, + num_sectors: u32, + flags: u32, +} +// Safe because DiscardWriteData only contains plain data. +unsafe impl ByteValued for DiscardWriteData {} + pub struct BlockWorker { queue: Queue, queue_evt: EventFd, @@ -247,6 +257,21 @@ impl BlockWorker { Ok(disk_id.len()) } } + VIRTIO_BLK_T_DISCARD => { + let discard_write_data: DiscardWriteData = reader + .read_obj() + .map_err(RequestError::ReadingFromDescriptor)?; + self.disk + .file + .lock() + .unwrap() + .discard_to_any( + discard_write_data.sector * 512, + discard_write_data.num_sectors as u64 * 512, + ) + .map_err(RequestError::Discarding)?; + Ok(0) + } _ => Err(RequestError::UnknownRequest), } } From 9149ac466ed93219655b9f3ae21a8d4dcb0dfab7 Mon Sep 17 00:00:00 2001 From: Sergio Lopez Date: Wed, 12 Nov 2025 18:29:54 +0100 Subject: [PATCH 5/5] virtio/block: implement support for F_WRITE_ZEROES The F_WRITE_ZEROES feature enables the guest to instruct the host to write zeroes to the disk image without actually having to copy any data around. Since imago does the heavy lifting for us, we just need to announce the feature and call to the right method when needed. Signed-off-by: Sergio Lopez --- src/devices/src/virtio/block/device.rs | 4 ++++ src/devices/src/virtio/block/worker.rs | 30 ++++++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/src/devices/src/virtio/block/device.rs b/src/devices/src/virtio/block/device.rs index 13d8992bb..5ba02319e 100644 --- a/src/devices/src/virtio/block/device.rs +++ b/src/devices/src/virtio/block/device.rs @@ -277,6 +277,7 @@ impl Block { | (1u64 << VIRTIO_BLK_F_FLUSH) | (1u64 << VIRTIO_BLK_F_SEG_MAX) | (1u64 << VIRTIO_BLK_F_DISCARD) + | (1u64 << VIRTIO_BLK_F_WRITE_ZEROES) | (1u64 << VIRTIO_RING_F_EVENT_IDX); if is_disk_read_only { @@ -295,6 +296,9 @@ impl Block { max_discard_sectors: u32::MAX, max_discard_seg: 1, discard_sector_alignment: discard_alignment as u32 / 512, + max_write_zeroes_sectors: u32::MAX, + max_write_zeroes_seg: 1, + write_zeroes_may_unmap: 1, ..Default::default() }; diff --git a/src/devices/src/virtio/block/worker.rs b/src/devices/src/virtio/block/worker.rs index 890e8995c..195a7d22b 100644 --- a/src/devices/src/virtio/block/worker.rs +++ b/src/devices/src/virtio/block/worker.rs @@ -17,10 +17,12 @@ use vm_memory::{ByteValued, GuestMemoryMmap}; #[derive(Debug)] pub enum RequestError { Discarding(io::Error), + DiscardingToZero(io::Error), FlushingToDisk(io::Error), InvalidDataLength, ReadingFromDescriptor(io::Error), WritingToDescriptor(io::Error), + WritingZeroes(io::Error), UnknownRequest, } @@ -272,6 +274,34 @@ impl BlockWorker { .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; + if unmap { + self.disk + .file + .lock() + .unwrap() + .discard_to_zero( + discard_write_data.sector * 512, + discard_write_data.num_sectors as u64 * 512, + ) + .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, + ) + .map_err(RequestError::WritingZeroes)?; + } + Ok(0) + } _ => Err(RequestError::UnknownRequest), } }