From 5176a822af8f05f0adef6ef52ba242ae9b5d7c29 Mon Sep 17 00:00:00 2001 From: Jake Correnti Date: Tue, 19 Nov 2024 10:17:35 -0500 Subject: [PATCH] Use SyncFormatAccess in DiskProperties Changes the `file` attribute in `DiskProperties` to use `Arc>`. 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 --- src/devices/src/virtio/block/device.rs | 63 +++++++++++++++++--------- src/devices/src/virtio/block/mod.rs | 7 +++ src/devices/src/virtio/block/worker.rs | 8 ++-- src/devices/src/virtio/file_traits.rs | 43 ++++++++++++++++++ src/libkrun/src/lib.rs | 5 ++ src/vmm/src/vmm_config/block.rs | 4 +- 6 files changed, 104 insertions(+), 26 deletions(-) diff --git a/src/devices/src/virtio/block/device.rs b/src/devices/src/virtio/block/device.rs index 893e26f3..f8038a51 100644 --- a/src/devices/src/virtio/block/device.rs +++ b/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>, nsectors: u64, image_id: Vec, } impl DiskProperties { pub fn new( - disk_image_path: String, - is_disk_read_only: bool, + disk_image: Arc>, + disk_image_id: Vec, cache_type: CacheType, ) -> io::Result { - 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 { + 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, cache_type: CacheType, - disk_image_path: String, - is_disk_read_only: bool, + disk_image: Arc>, + disk_image_id: Vec, worker_thread: Option>, worker_stopfd: EventFd, @@ -203,10 +202,32 @@ impl Block { partuuid: Option, cache_type: CacheType, disk_image_path: String, + disk_image_format: ImageType, is_disk_read_only: bool, ) -> io::Result { + 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::::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)?, diff --git a/src/devices/src/virtio/block/mod.rs b/src/devices/src/virtio/block/mod.rs index e7f81354..7fe4fb4d 100644 --- a/src/devices/src/virtio/block/mod.rs +++ b/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, +} diff --git a/src/devices/src/virtio/block/worker.rs b/src/devices/src/virtio/block/worker.rs index 1590ee13..96d14314 100644 --- a/src/devices/src/virtio/block/worker.rs +++ b/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), diff --git a/src/devices/src/virtio/file_traits.rs b/src/devices/src/virtio/file_traits.rs index 00aef854..2e13eb67 100644 --- a/src/devices/src/virtio/file_traits.rs +++ b/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 { + self.read_vectored_at_volatile(&[slice], offset) + } + + fn read_vectored_at_volatile(&self, bufs: &[VolatileSlice], offset: u64) -> Result { + 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 { + self.write_vectored_at_volatile(&[slice], offset) + } + + fn write_vectored_at_volatile(&self, bufs: &[VolatileSlice], offset: u64) -> Result { + 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) + } +} diff --git a/src/libkrun/src/lib.rs b/src/libkrun/src/lib.rs index 77c84d5d..058353fe 100644 --- a/src/libkrun/src/lib.rs +++ b/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); diff --git a/src/vmm/src/vmm_config/block.rs b/src/vmm/src/vmm_config/block.rs index 81f31184..25bd20da 100644 --- a/src/vmm/src/vmm_config/block.rs +++ b/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)