From 2d01677a6538fd4f682d0218759d1d00bbe5e22e Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Wed, 10 Aug 2022 16:03:29 -0700 Subject: [PATCH] disk: remove non-async disk image support With AsyncDiskFileWrapper, we can use all disk image formats via the async API. The create_async_disk_file() function is removed, and the existing create_disk_file() function can be used to create async-capable disk files, as the DiskFile trait now includes ToAsyncDisk. BUG=b:219595052 TEST=cargo build --features=composite-disk TEST=Boot x86-64 Linux in crosvm with a qcow2 disk attached Change-Id: I430d306fe71c325ab529a7031353d20f8a9aa2f3 Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3824067 Tested-by: Daniel Verkamp Reviewed-by: Noah Gold Reviewed-by: Alexandre Courbot Commit-Queue: Daniel Verkamp --- devices/src/virtio/block/asynchronous.rs | 8 +-- devices/src/virtio/block/sys/unix.rs | 17 ++---- .../vhost/user/device/block/sys/unix.rs | 2 +- .../vhost/user/device/block/sys/windows.rs | 5 -- disk/src/asynchronous.rs | 5 +- disk/src/disk.rs | 26 ++------ src/crosvm/sys/unix/device_helpers.rs | 61 ++++++------------- 7 files changed, 36 insertions(+), 88 deletions(-) diff --git a/devices/src/virtio/block/asynchronous.rs b/devices/src/virtio/block/asynchronous.rs index 71c0b4e4fc..a33234ee67 100644 --- a/devices/src/virtio/block/asynchronous.rs +++ b/devices/src/virtio/block/asynchronous.rs @@ -36,7 +36,7 @@ use cros_async::SelectResult; use cros_async::TimerAsync; use data_model::DataInit; use disk::AsyncDisk; -use disk::ToAsyncDisk; +use disk::DiskFile; use futures::pin_mut; use futures::stream::FuturesUnordered; use futures::stream::StreamExt; @@ -502,7 +502,7 @@ fn run_worker( /// Virtio device for exposing block level read/write operations on a host file. pub struct BlockAsync { // We keep these members crate-public as they are accessed by the vhost-user device. - pub(crate) disk_image: Option>, + pub(crate) disk_image: Option>, pub(crate) disk_size: Arc, pub(crate) avail_features: u64, pub(crate) read_only: bool, @@ -512,14 +512,14 @@ pub struct BlockAsync { pub(crate) id: Option, pub(crate) control_tube: Option, kill_evt: Option, - worker_thread: Option, Option)>>, + worker_thread: Option, Option)>>, } impl BlockAsync { /// Create a new virtio block device that operates on the given AsyncDisk. pub fn new( base_features: u64, - disk_image: Box, + disk_image: Box, read_only: bool, sparse: bool, block_size: u32, diff --git a/devices/src/virtio/block/sys/unix.rs b/devices/src/virtio/block/sys/unix.rs index 8d3a0e14a4..54cd70778b 100644 --- a/devices/src/virtio/block/sys/unix.rs +++ b/devices/src/virtio/block/sys/unix.rs @@ -13,7 +13,7 @@ use base::flock; use base::iov_max; use base::open_file; use base::FlockOperation; -use disk::ToAsyncDisk; +use disk::DiskFile; use crate::virtio::block::block::DiskOption; @@ -27,8 +27,8 @@ pub fn get_seg_max(queue_size: u16) -> u32 { } impl DiskOption { - /// Open the specified disk file as a raw image. - pub fn open_as_raw_image(&self) -> anyhow::Result { + /// Open the specified disk file. + pub fn open(&self) -> anyhow::Result> { let mut options = OpenOptions::new(); options.read(true).write(!self.read_only); @@ -47,14 +47,7 @@ impl DiskOption { flock(&raw_image, lock_op, true) .with_context(|| format!("failed to lock disk image {}", self.path.display()))?; - Ok(raw_image) - } - - pub fn open_as_async_file(&self) -> anyhow::Result> { - let raw_image = self.open_as_raw_image()?; - let async_file = disk::create_async_disk_file(raw_image) - .context("failed to create async virtual disk")?; - - Ok(async_file) + disk::create_disk_file(raw_image, self.sparse, disk::MAX_NESTING_DEPTH, &self.path) + .context("create_disk_file failed") } } diff --git a/devices/src/virtio/vhost/user/device/block/sys/unix.rs b/devices/src/virtio/vhost/user/device/block/sys/unix.rs index 84e4b6d6f7..a99d224af0 100644 --- a/devices/src/virtio/vhost/user/device/block/sys/unix.rs +++ b/devices/src/virtio/vhost/user/device/block/sys/unix.rs @@ -53,7 +53,7 @@ pub fn start_device(opts: Options) -> anyhow::Result<()> { let block = Box::new(BlockAsync::new( base_features(ProtectionType::Unprotected), - disk.open_as_async_file()?, + disk.open()?, disk.read_only, disk.sparse, disk.block_size, diff --git a/devices/src/virtio/vhost/user/device/block/sys/windows.rs b/devices/src/virtio/vhost/user/device/block/sys/windows.rs index d3423319a7..08d0772650 100644 --- a/devices/src/virtio/vhost/user/device/block/sys/windows.rs +++ b/devices/src/virtio/vhost/user/device/block/sys/windows.rs @@ -16,7 +16,6 @@ use base::RawDescriptor; use broker_ipc::common_child_setup; use broker_ipc::CommonChildStartupArgs; use cros_async::Executor; -use disk::async_ok; use disk::AsyncDisk; use disk::SingleFileDisk; use hypervisor::ProtectionType; @@ -117,10 +116,6 @@ pub fn start_device(opts: Options) -> anyhow::Result<()> { files.push(open_disk_file(&disk_option, take_write_lock)?); } - if !async_ok(&files[0])? { - bail!("found non-raw image; only raw images are supported."); - } - let base_features = base_features(ProtectionType::Unprotected); let ex = Executor::new().context("failed to create executor")?; diff --git a/disk/src/asynchronous.rs b/disk/src/asynchronous.rs index e5b137d36b..1f6f26e65a 100644 --- a/disk/src/asynchronous.rs +++ b/disk/src/asynchronous.rs @@ -23,7 +23,6 @@ use crate::DiskFile; use crate::DiskGetLen; use crate::Error; use crate::Result; -use crate::ToAsyncDisk; /// Async wrapper around a non-async `DiskFile` using a `BlockingPool`. /// @@ -68,8 +67,8 @@ impl AsRawDescriptors for AsyncDiskFileWrapper { } #[async_trait(?Send)] -impl AsyncDisk for AsyncDiskFileWrapper { - fn into_inner(self: Box) -> Box { +impl AsyncDisk for AsyncDiskFileWrapper { + fn into_inner(self: Box) -> Box { self.blocking_pool .shutdown(None) .expect("AsyncDiskFile pool shutdown failed"); diff --git a/disk/src/disk.rs b/disk/src/disk.rs index 3b95e81ed7..2fe8cfb4ec 100644 --- a/disk/src/disk.rs +++ b/disk/src/disk.rs @@ -151,6 +151,7 @@ pub trait DiskFile: + PunchHole + WriteZeroesAt + FileAllocate + + ToAsyncDisk + Send + AsRawDescriptors + Debug @@ -164,6 +165,7 @@ impl< + FileReadWriteAtVolatile + WriteZeroesAt + FileAllocate + + ToAsyncDisk + Send + AsRawDescriptors + Debug, @@ -253,26 +255,6 @@ pub fn detect_image_type(file: &File) -> Result { Ok(ImageType::Raw) } -/// Check if the image file type can be used for async disk access. -pub fn async_ok(raw_image: &File) -> Result { - let image_type = detect_image_type(raw_image)?; - Ok(match image_type { - ImageType::Raw => true, - ImageType::Qcow2 | ImageType::AndroidSparse | ImageType::CompositeDisk => false, - }) -} - -/// Inspect the image file type and create an appropriate disk file to match it. -pub fn create_async_disk_file(raw_image: File) -> Result> { - let image_type = detect_image_type(&raw_image)?; - Ok(match image_type { - ImageType::Raw => Box::new(raw_image) as Box, - ImageType::Qcow2 | ImageType::AndroidSparse | ImageType::CompositeDisk => { - return Err(Error::UnknownType) - } - }) -} - /// Inspect the image file type and create an appropriate disk file to match it. pub fn create_disk_file( raw_image: File, @@ -327,7 +309,7 @@ pub fn create_disk_file( #[async_trait(?Send)] pub trait AsyncDisk: DiskGetLen + FileSetLen + FileAllocate { /// Returns the inner file consuming self. - fn into_inner(self: Box) -> Box; + fn into_inner(self: Box) -> Box; /// Asynchronously fsyncs any completed operations to the disk. async fn fsync(&self) -> Result<()>; @@ -389,7 +371,7 @@ impl FileAllocate for SingleFileDisk { #[async_trait(?Send)] impl AsyncDisk for SingleFileDisk { - fn into_inner(self: Box) -> Box { + fn into_inner(self: Box) -> Box { Box::new(self.inner.into_source()) } diff --git a/src/crosvm/sys/unix/device_helpers.rs b/src/crosvm/sys/unix/device_helpers.rs index b947919132..5c46c6e637 100644 --- a/src/crosvm/sys/unix/device_helpers.rs +++ b/src/crosvm/sys/unix/device_helpers.rs @@ -240,46 +240,25 @@ impl<'a> VirtioDeviceBuilder for DiskConfig<'a> { &self, protection_type: ProtectionType, ) -> anyhow::Result> { - let disk = self.disk; + info!( + "Trying to attach block device: {}", + self.disk.path.display(), + ); + let disk_image = self.disk.open()?; + let disk_device_tube = self.device_tube.take(); - - let raw_image = disk.open_as_raw_image()?; - - info!("Trying to attach block device: {}", disk.path.display()); - let dev = if disk::async_ok(&raw_image).context("failed to check disk async_ok")? { - let async_file = disk::create_async_disk_file(raw_image) - .context("failed to create async virtual disk")?; - Box::new( - virtio::BlockAsync::new( - virtio::base_features(protection_type), - async_file, - disk.read_only, - disk.sparse, - disk.block_size, - disk.id, - disk_device_tube, - ) - .context("failed to create block device")?, - ) as Box - } else { - let disk_file = - disk::create_disk_file(raw_image, disk.sparse, disk::MAX_NESTING_DEPTH, &disk.path) - .context("failed to create virtual disk")?; - Box::new( - virtio::Block::new( - virtio::base_features(protection_type), - disk_file, - disk.read_only, - disk.sparse, - disk.block_size, - disk.id, - disk_device_tube, - ) - .context("failed to create block device")?, + Ok(Box::new( + virtio::BlockAsync::new( + virtio::base_features(protection_type), + disk_image, + self.disk.read_only, + self.disk.sparse, + self.disk.block_size, + self.disk.id, + disk_device_tube, ) - }; - - Ok(dev) + .context("failed to create block device")?, + )) } fn create_vhost_user_device( @@ -293,13 +272,13 @@ impl<'a> VirtioDeviceBuilder for DiskConfig<'a> { keep_rds.push(device_tube.as_raw_descriptor()); } - let async_file = disk.open_as_async_file()?; - keep_rds.extend(async_file.as_raw_descriptors()); + let disk_image = disk.open()?; + keep_rds.extend(disk_image.as_raw_descriptors()); let block = Box::new( virtio::BlockAsync::new( virtio::base_features(ProtectionType::Unprotected), - async_file, + disk_image, disk.read_only, disk.sparse, disk.block_size,