Use SyncFormatAccess<ImagoFile> in DiskProperties

Changes the `file` attribute in `DiskProperties` to use
`Arc<SyncFormatAccess<ImagoFile>>`. This allows libkrun to take
advantage of imago's support for multiple disk image formats. Libkrun
now supports the use of Raw or QCOW2 formats for disk images.

Implements `FileReadWriteAtVolatile` trait on `DiskProperties`

Adds an emum, `ImageType`, that describes the supported disk image
variants.

Signed-off-by: Jake Correnti <[email protected]>

Author: jakecorrenti

src/devices/src/virtio/block/device.rs

@@ -8,7 +8,7 @@
 use std::cmp;
 use std::convert::From;
 use std::fs::{File, OpenOptions};
-use std::io::{self, Seek, SeekFrom, Write};
+use std::io::{self, Write};
 #[cfg(target_os = "linux")]
 use std::os::linux::fs::MetadataExt;
 #[cfg(target_os = "macos")]
@@ -19,6 +19,9 @@ use std::sync::atomic::AtomicUsize;
 use std::sync::{Arc, Mutex};
 use std::thread::JoinHandle;
 
+use imago::file::File as ImagoFile;
+use imago::qcow2::Qcow2;
+use imago::SyncFormatAccess;
 use log::{error, warn};
 use utils::eventfd::{EventFd, EFD_NONBLOCK};
 use virtio_bindings::{
@@ -33,7 +36,7 @@ use super::{
 };
 
 use crate::legacy::Gic;
-use crate::virtio::ActivateError;
+use crate::virtio::{block::ImageType, ActivateError};
 
 /// Configuration options for disk caching.
 #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
@@ -51,22 +54,18 @@ pub enum CacheType {
 /// Helper object for setting up all `Block` fields derived from its backing file.
 pub(crate) struct DiskProperties {
     cache_type: CacheType,
-    pub(crate) file: File,
+    pub(crate) file: Arc<SyncFormatAccess<ImagoFile>>,
     nsectors: u64,
     image_id: Vec<u8>,
 }
 
 impl DiskProperties {
     pub fn new(
-        disk_image_path: String,
-        is_disk_read_only: bool,
+        disk_image: Arc<SyncFormatAccess<ImagoFile>>,
+        disk_image_id: Vec<u8>,
         cache_type: CacheType,
     ) -> io::Result<Self> {
-        let mut disk_image = OpenOptions::new()
-            .read(true)
-            .write(!is_disk_read_only)
-            .open(PathBuf::from(&disk_image_path))?;
-        let disk_size = disk_image.seek(SeekFrom::End(0))?;
+        let disk_size = disk_image.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.
@@ -81,13 +80,13 @@ impl DiskProperties {
         Ok(Self {
             cache_type,
             nsectors: disk_size >> SECTOR_SHIFT,
-            image_id: Self::build_disk_image_id(&disk_image),
+            image_id: disk_image_id,
             file: disk_image,
         })
     }
 
-    pub fn file_mut(&mut self) -> &mut File {
-        &mut self.file
+    pub fn file(&self) -> &SyncFormatAccess<ImagoFile> {
+        self.file.as_ref()
     }
 
     pub fn nsectors(&self) -> u64 {
@@ -141,7 +140,7 @@ impl Drop for DiskProperties {
                     error!("Failed to flush block data on drop.");
                 }
                 // Sync data out to physical media on host.
-                if self.file.sync_all().is_err() {
+                if self.file.sync().is_err() {
                     error!("Failed to sync block data on drop.")
                 }
             }
@@ -168,8 +167,8 @@ pub struct Block {
     // Host file and properties.
     disk: Option<DiskProperties>,
     cache_type: CacheType,
-    disk_image_path: String,
-    is_disk_read_only: bool,
+    disk_image: Arc<SyncFormatAccess<ImagoFile>>,
+    disk_image_id: Vec<u8>,
     worker_thread: Option<JoinHandle<()>>,
     worker_stopfd: EventFd,
 
@@ -203,10 +202,32 @@ impl Block {
         partuuid: Option<String>,
         cache_type: CacheType,
         disk_image_path: String,
+        disk_image_format: ImageType,
         is_disk_read_only: bool,
     ) -> io::Result<Block> {
+        let disk_image = OpenOptions::new()
+            .read(true)
+            .write(!is_disk_read_only)
+            .open(PathBuf::from(&disk_image_path))?;
+
+        let disk_image_id = DiskProperties::build_disk_image_id(&disk_image);
+
+        let disk_image = match disk_image_format {
+            ImageType::Qcow2 => {
+                let mut qcow_disk_image =
+                    Qcow2::<ImagoFile>::open_path_sync(disk_image_path, !is_disk_read_only)?;
+                qcow_disk_image.open_implicit_dependencies_sync()?;
+                SyncFormatAccess::new(qcow_disk_image)?
+            }
+            ImageType::Raw => {
+                let raw = imago::raw::Raw::open_path_sync(disk_image_path, !is_disk_read_only)?;
+                SyncFormatAccess::new(raw)?
+            }
+        };
+        let disk_image = Arc::new(disk_image);
+
         let disk_properties =
-            DiskProperties::new(disk_image_path.clone(), is_disk_read_only, cache_type)?;
+            DiskProperties::new(Arc::clone(&disk_image), disk_image_id.clone(), cache_type)?;
 
         let mut avail_features = (1u64 << VIRTIO_F_VERSION_1)
             | (1u64 << VIRTIO_BLK_F_FLUSH)
@@ -234,8 +255,8 @@ impl Block {
             config,
             disk: Some(disk_properties),
             cache_type,
-            disk_image_path,
-            is_disk_read_only,
+            disk_image,
+            disk_image_id,
             avail_features,
             acked_features: 0u64,
             interrupt_status: Arc::new(AtomicUsize::new(0)),
@@ -348,8 +369,8 @@ impl VirtioDevice for Block {
         let disk = match self.disk.take() {
             Some(d) => d,
             None => DiskProperties::new(
-                self.disk_image_path.clone(),
-                self.is_disk_read_only,
+                Arc::clone(&self.disk_image),
+                self.disk_image_id.clone(),
                 self.cache_type,
             )
             .map_err(|_| ActivateError::BadActivate)?,

src/devices/src/virtio/block/mod.rs

@@ -32,3 +32,10 @@ pub enum Error {
     /// Guest gave us a write only descriptor that protocol says to read from.
     UnexpectedWriteOnlyDescriptor,
 }
+
+/// Supported disk image formats
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum ImageType {
+    Raw,
+    Qcow2,
+}

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

@@ -228,7 +228,7 @@ impl BlockWorker {
                     Err(RequestError::InvalidDataLength)
                 } else {
                     writer
-                        .write_from_at(&self.disk.file, data_len, request_header.sector * 512)
+                        .write_from_at(&self.disk, data_len, request_header.sector * 512)
                         .map_err(RequestError::WritingToDescriptor)
                 }
             }
@@ -238,15 +238,15 @@ impl BlockWorker {
                     Err(RequestError::InvalidDataLength)
                 } else {
                     reader
-                        .read_to_at(&self.disk.file, data_len, request_header.sector * 512)
+                        .read_to_at(&self.disk, data_len, request_header.sector * 512)
                         .map_err(RequestError::ReadingFromDescriptor)
                 }
             }
             VIRTIO_BLK_T_FLUSH => match self.disk.cache_type() {
                 CacheType::Writeback => {
-                    let diskfile = self.disk.file_mut();
+                    let diskfile = self.disk.file();
                     diskfile.flush().map_err(RequestError::FlushingToDisk)?;
-                    diskfile.sync_all().map_err(RequestError::FlushingToDisk)?;
+                    diskfile.sync().map_err(RequestError::FlushingToDisk)?;
                     Ok(0)
                 }
                 CacheType::Unsafe => Ok(0),

src/devices/src/virtio/file_traits.rs

@@ -6,11 +6,15 @@ use std::fs::File;
 use std::io::{Error, ErrorKind, Result};
 use std::os::unix::io::AsRawFd;
 
+#[cfg(feature = "blk")]
+use imago::io_buffers::{IoVector, IoVectorMut};
 use vm_memory::VolatileSlice;
 
 use libc::{c_int, c_void, read, readv, size_t, write, writev};
 
 use super::bindings::{off64_t, pread64, preadv64, pwrite64, pwritev64};
+#[cfg(feature = "blk")]
+use super::block::device::DiskProperties;
 
 /// A trait for setting the size of a file.
 /// This is equivalent to File's `set_len` method, but
@@ -411,3 +415,42 @@ macro_rules! volatile_impl {
 }
 
 volatile_impl!(File);
+
+#[cfg(feature = "blk")]
+impl FileReadWriteAtVolatile for DiskProperties {
+    fn read_at_volatile(&self, slice: VolatileSlice, offset: u64) -> Result<usize> {
+        self.read_vectored_at_volatile(&[slice], offset)
+    }
+
+    fn read_vectored_at_volatile(&self, bufs: &[VolatileSlice], offset: u64) -> Result<usize> {
+        if bufs.is_empty() {
+            return Ok(0);
+        }
+
+        let (iovec, _guard) = IoVectorMut::from_volatile_slice(bufs);
+        let full_length = iovec
+            .len()
+            .try_into()
+            .map_err(|e| Error::new(ErrorKind::InvalidData, e))?;
+        self.file().readv(iovec, offset)?;
+        Ok(full_length)
+    }
+
+    fn write_at_volatile(&self, slice: VolatileSlice, offset: u64) -> Result<usize> {
+        self.write_vectored_at_volatile(&[slice], offset)
+    }
+
+    fn write_vectored_at_volatile(&self, bufs: &[VolatileSlice], offset: u64) -> Result<usize> {
+        if bufs.is_empty() {
+            return Ok(0);
+        }
+
+        let (iovec, _guard) = IoVector::from_volatile_slice(bufs);
+        let full_length = iovec
+            .len()
+            .try_into()
+            .map_err(|e| Error::new(ErrorKind::InvalidData, e))?;
+        self.file().writev(iovec, offset)?;
+        Ok(full_length)
+    }
+}

src/libkrun/src/lib.rs

@@ -18,6 +18,8 @@ use std::sync::Mutex;
 
 #[cfg(target_os = "macos")]
 use crossbeam_channel::unbounded;
+#[cfg(feature = "blk")]
+use devices::virtio::block::ImageType;
 #[cfg(feature = "net")]
 use devices::virtio::net::device::VirtioNetBackend;
 #[cfg(feature = "blk")]
@@ -521,6 +523,7 @@ pub unsafe extern "C" fn krun_add_disk(
                 block_id: block_id.to_string(),
                 cache_type: CacheType::Writeback,
                 disk_image_path: disk_path.to_string(),
+                disk_image_format: ImageType::Raw,
                 is_disk_read_only: read_only,
             };
             cfg.add_block_cfg(block_device_config);
@@ -547,6 +550,7 @@ pub unsafe extern "C" fn krun_set_root_disk(ctx_id: u32, c_disk_path: *const c_c
                 block_id: "root".to_string(),
                 cache_type: CacheType::Writeback,
                 disk_image_path: disk_path.to_string(),
+                disk_image_format: ImageType::Raw,
                 is_disk_read_only: false,
             };
             cfg.set_root_block_cfg(block_device_config);
@@ -573,6 +577,7 @@ pub unsafe extern "C" fn krun_set_data_disk(ctx_id: u32, c_disk_path: *const c_c
                 block_id: "data".to_string(),
                 cache_type: CacheType::Writeback,
                 disk_image_path: disk_path.to_string(),
+                disk_image_format: ImageType::Raw,
                 is_disk_read_only: false,
             };
             cfg.set_data_block_cfg(block_device_config);

src/vmm/src/vmm_config/block.rs

@@ -2,7 +2,7 @@ use std::collections::VecDeque;
 use std::fmt;
 use std::sync::{Arc, Mutex};
 
-use devices::virtio::{Block, CacheType};
+use devices::virtio::{block::ImageType, Block, CacheType};
 
 #[derive(Debug)]
 pub enum BlockConfigError {
@@ -26,6 +26,7 @@ pub struct BlockDeviceConfig {
     pub block_id: String,
     pub cache_type: CacheType,
     pub disk_image_path: String,
+    pub disk_image_format: ImageType,
     pub is_disk_read_only: bool,
 }
 
@@ -53,6 +54,7 @@ impl BlockBuilder {
             None,
             config.cache_type,
             config.disk_image_path,
+            config.disk_image_format,
             config.is_disk_read_only,
         )
         .map_err(BlockConfigError::CreateBlockDevice)