Implement support for both F_DISCARD and F_WRITE_ZEROES

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" }

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<SyncFormatAccess<Box<dyn DynStorage>>>,
+    pub(crate) file: Arc<Mutex<SyncFormatAccess<Box<dyn DynStorage>>>>,
     nsectors: u64,
     image_id: Vec<u8>,
 }
 
 impl DiskProperties {
     pub fn new(
-        disk_image: Arc<SyncFormatAccess<Box<dyn DynStorage>>>,
+        disk_image: Arc<Mutex<SyncFormatAccess<Box<dyn DynStorage>>>>,
         disk_image_id: Vec<u8>,
         cache_type: CacheType,
     ) -> io::Result<Self> {
-        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<Box<dyn DynStorage>> {
-        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.")
                 }
             }
@@ -161,12 +157,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.
@@ -177,7 +202,7 @@ pub struct Block {
     // Host file and properties.
     disk: Option<DiskProperties>,
     cache_type: CacheType,
-    disk_image: Arc<SyncFormatAccess<Box<dyn DynStorage>>>,
+    disk_image: Arc<Mutex<SyncFormatAccess<Box<dyn DynStorage>>>>,
     disk_image_id: Vec<u8>,
     worker_thread: Option<JoinHandle<()>>,
     worker_stopfd: EventFd,
@@ -222,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 => {
@@ -242,14 +268,16 @@ 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)
             | (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 {
@@ -265,6 +293,13 @@ 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,
+            max_write_zeroes_sectors: u32::MAX,
+            max_write_zeroes_seg: 1,
+            write_zeroes_may_unmap: 1,
+            ..Default::default()
         };
 
         Ok(Block {

src/devices/src/virtio/block/worker.rs

@@ -16,10 +16,13 @@ use vm_memory::{ByteValued, GuestMemoryMmap};
 #[allow(dead_code)]
 #[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,
 }
 
@@ -39,10 +42,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,
@@ -228,7 +240,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)
@@ -247,6 +259,49 @@ 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)
+            }
+            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),
         }
     }

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)
     }
 }