fix: patching the path of Async block devices

The asynchronous engine maintains an event file descriptor which passes
to the IO uring interface when creating a new ring. IO uring uses this
EventFd to notify us about completion of IO requests.

When we PATCH an async block device, we create a new asynchronous
engine, including a new EventFd. However, we still monitor the old
EventFd. This breaks the use of async drives post PATCH requests,
because we never get notified about the results of requests we submit to
the IO uring engine.

This commit changes the implementation along the PATCH code path, to
reuse the previous EventFd for the asynchronous engine.

Signed-off-by: Babis Chalios <[email protected]>

Author: bchalios

src/vmm/src/devices/virtio/virtio_block/device.rs

@@ -71,19 +71,20 @@ pub struct DiskProperties {
 }
 
 impl DiskProperties {
-    pub fn new(
-        disk_image_path: String,
-        is_disk_read_only: bool,
-        file_engine_type: FileEngineType,
-    ) -> Result<Self, VirtioBlockError> {
-        let mut disk_image = OpenOptions::new()
+    // Helper function that opens the file with the proper access permissions
+    fn open_file(disk_image_path: &str, is_disk_read_only: bool) -> Result<File, VirtioBlockError> {
+        OpenOptions::new()
             .read(true)
             .write(!is_disk_read_only)
             .open(PathBuf::from(&disk_image_path))
-            .map_err(|x| VirtioBlockError::BackingFile(x, disk_image_path.clone()))?;
+            .map_err(|x| VirtioBlockError::BackingFile(x, disk_image_path.to_string()))
+    }
+
+    // Helper function that gets the size of the file
+    fn file_size(disk_image_path: &str, disk_image: &mut File) -> Result<u64, VirtioBlockError> {
         let disk_size = disk_image
             .seek(SeekFrom::End(0))
-            .map_err(|x| VirtioBlockError::BackingFile(x, disk_image_path.clone()))?;
+            .map_err(|x| VirtioBlockError::BackingFile(x, disk_image_path.to_string()))?;
 
         // 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,6 +96,17 @@ impl DiskProperties {
             );
         }
 
+        Ok(disk_size)
+    }
+
+    /// Create a new file for the block device using a FileEngine
+    pub fn new(
+        disk_image_path: String,
+        is_disk_read_only: bool,
+        file_engine_type: FileEngineType,
+    ) -> Result<Self, VirtioBlockError> {
+        let mut disk_image = Self::open_file(&disk_image_path, is_disk_read_only)?;
+        let disk_size = Self::file_size(&disk_image_path, &mut disk_image)?;
         let image_id = Self::build_disk_image_id(&disk_image);
 
         Ok(Self {
@@ -106,6 +118,25 @@ impl DiskProperties {
         })
     }
 
+    /// Update the path to the file backing the block device
+    pub fn update(
+        &mut self,
+        disk_image_path: String,
+        is_disk_read_only: bool,
+    ) -> Result<(), VirtioBlockError> {
+        let mut disk_image = Self::open_file(&disk_image_path, is_disk_read_only)?;
+        let disk_size = Self::file_size(&disk_image_path, &mut disk_image)?;
+
+        self.image_id = Self::build_disk_image_id(&disk_image);
+        self.file_engine
+            .update_file_path(disk_image)
+            .map_err(VirtioBlockError::FileEngine)?;
+        self.nsectors = disk_size >> SECTOR_SHIFT;
+        self.file_path = disk_image_path;
+
+        Ok(())
+    }
+
     fn build_device_id(disk_file: &File) -> Result<String, VirtioBlockError> {
         let blk_metadata = disk_file
             .metadata()
@@ -506,9 +537,7 @@ impl VirtioBlock {
 
     /// Update the backing file and the config space of the block device.
     pub fn update_disk_image(&mut self, disk_image_path: String) -> Result<(), VirtioBlockError> {
-        let disk_properties =
-            DiskProperties::new(disk_image_path, self.read_only, self.file_engine_type())?;
-        self.disk = disk_properties;
+        self.disk.update(disk_image_path, self.read_only)?;
         self.config_space = self.disk.virtio_block_config_space();
 
         // Kick the driver to pick up the changes.

src/vmm/src/devices/virtio/virtio_block/io/async_io.rs

@@ -4,6 +4,7 @@
 use std::fmt::Debug;
 use std::fs::File;
 use std::marker::PhantomData;
+use std::os::fd::RawFd;
 use std::os::unix::io::AsRawFd;
 
 use utils::eventfd::EventFd;
@@ -13,7 +14,7 @@ use crate::devices::virtio::virtio_block::io::UserDataError;
 use crate::devices::virtio::virtio_block::IO_URING_NUM_ENTRIES;
 use crate::io_uring::operation::{Cqe, OpCode, Operation};
 use crate::io_uring::restriction::Restriction;
-use crate::io_uring::{IoUring, IoUringError};
+use crate::io_uring::{self, IoUring, IoUringError};
 use crate::logger::log_dev_preview_warning;
 use crate::vstate::memory::{GuestAddress, GuestMemory, GuestMemoryExtension, GuestMemoryMmap};
 
@@ -66,13 +67,10 @@ impl<T: Debug> WrappedUserData<T> {
 }
 
 impl<T: Debug> AsyncFileEngine<T> {
-    pub fn from_file(file: File) -> Result<AsyncFileEngine<T>, AsyncIoError> {
-        log_dev_preview_warning("Async file IO", Option::None);
-
-        let completion_evt = EventFd::new(libc::EFD_NONBLOCK).map_err(AsyncIoError::EventFd)?;
-        let ring = IoUring::new(
+    fn new_ring(file: &File, completion_fd: RawFd) -> Result<IoUring, io_uring::IoUringError> {
+        IoUring::new(
             u32::from(IO_URING_NUM_ENTRIES),
-            vec![&file],
+            vec![file],
             vec![
                 // Make sure we only allow operations on pre-registered fds.
                 Restriction::RequireFixedFds,
@@ -81,9 +79,16 @@ impl<T: Debug> AsyncFileEngine<T> {
                 Restriction::AllowOpCode(OpCode::Write),
                 Restriction::AllowOpCode(OpCode::Fsync),
             ],
-            Some(completion_evt.as_raw_fd()),
+            Some(completion_fd),
         )
-        .map_err(AsyncIoError::IoUring)?;
+    }
+
+    pub fn from_file(file: File) -> Result<AsyncFileEngine<T>, AsyncIoError> {
+        log_dev_preview_warning("Async file IO", Option::None);
+
+        let completion_evt = EventFd::new(libc::EFD_NONBLOCK).map_err(AsyncIoError::EventFd)?;
+        let ring =
+            Self::new_ring(&file, completion_evt.as_raw_fd()).map_err(AsyncIoError::IoUring)?;
 
         Ok(AsyncFileEngine {
             file,
@@ -93,6 +98,15 @@ impl<T: Debug> AsyncFileEngine<T> {
         })
     }
 
+    pub fn update_file(&mut self, file: File) -> Result<(), AsyncIoError> {
+        let ring = Self::new_ring(&file, self.completion_evt.as_raw_fd())
+            .map_err(AsyncIoError::IoUring)?;
+
+        self.file = file;
+        self.ring = ring;
+        Ok(())
+    }
+
     #[cfg(test)]
     pub fn file(&self) -> &File {
         &self.file

src/vmm/src/devices/virtio/virtio_block/io/mod.rs

@@ -74,6 +74,15 @@ impl<T: Debug> FileEngine<T> {
         }
     }
 
+    pub fn update_file_path(&mut self, file: File) -> Result<(), BlockIoError> {
+        match self {
+            FileEngine::Async(engine) => engine.update_file(file).map_err(BlockIoError::Async)?,
+            FileEngine::Sync(engine) => engine.update_file(file),
+        };
+
+        Ok(())
+    }
+
     #[cfg(test)]
     pub fn file(&self) -> &File {
         match self {

src/vmm/src/devices/virtio/virtio_block/io/sync_io.rs

@@ -34,6 +34,11 @@ impl SyncFileEngine {
         &self.file
     }
 
+    /// Update the backing file of the engine
+    pub fn update_file(&mut self, file: File) {
+        self.file = file
+    }
+
     pub fn read(
         &mut self,
         offset: u64,