From dbf0c8ca0d1d66c0a95f7aaef5ae44c50262975d Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 11 Apr 2025 11:49:38 +0100 Subject: [PATCH] refactor: de-generify async_io engine These generic type parameters were leaking out from the io_uring module. Having a generic ring structure in the io_uring module: makes sense. Having a generic file engine: probably makes less sense (what are we ever gonna process in there except block io requests?). Just replace all instances of `T` in the virtio module with `PendingRequest` - that's what it all ended up getting monomorphized to anyway. Signed-off-by: Patrick Roy --- .../src/devices/virtio/block/virtio/device.rs | 2 +- .../virtio/block/virtio/io/async_io.rs | 82 +++++----- .../src/devices/virtio/block/virtio/io/mod.rs | 154 +++++++++--------- .../devices/virtio/block/virtio/request.rs | 15 +- 4 files changed, 136 insertions(+), 117 deletions(-) diff --git a/src/vmm/src/devices/virtio/block/virtio/device.rs b/src/vmm/src/devices/virtio/block/virtio/device.rs index e2b134a9f25..b11c757d43c 100644 --- a/src/vmm/src/devices/virtio/block/virtio/device.rs +++ b/src/vmm/src/devices/virtio/block/virtio/device.rs @@ -52,7 +52,7 @@ pub enum FileEngineType { #[derive(Debug)] pub struct DiskProperties { pub file_path: String, - pub file_engine: FileEngine, + pub file_engine: FileEngine, pub nsectors: u64, pub image_id: [u8; VIRTIO_BLK_ID_BYTES as usize], } diff --git a/src/vmm/src/devices/virtio/block/virtio/io/async_io.rs b/src/vmm/src/devices/virtio/block/virtio/io/async_io.rs index 3073b55886a..f83d9dea1df 100644 --- a/src/vmm/src/devices/virtio/block/virtio/io/async_io.rs +++ b/src/vmm/src/devices/virtio/block/virtio/io/async_io.rs @@ -9,11 +9,11 @@ use std::os::unix::io::AsRawFd; use vm_memory::GuestMemoryError; use vmm_sys_util::eventfd::EventFd; -use crate::devices::virtio::block::virtio::IO_URING_NUM_ENTRIES; -use crate::devices::virtio::block::virtio::io::UserDataError; +use crate::devices::virtio::block::virtio::io::RequestError; +use crate::devices::virtio::block::virtio::{IO_URING_NUM_ENTRIES, PendingRequest}; use crate::io_uring::operation::{Cqe, OpCode, Operation}; use crate::io_uring::restriction::Restriction; -use crate::io_uring::{self, IoUring, IoUringError}; +use crate::io_uring::{IoUring, IoUringError}; use crate::logger::log_dev_preview_warning; use crate::vstate::memory::{GuestAddress, GuestMemory, GuestMemoryExtension, GuestMemoryMmap}; @@ -34,47 +34,44 @@ pub enum AsyncIoError { } #[derive(Debug)] -pub struct AsyncFileEngine { +pub struct AsyncFileEngine { file: File, - ring: IoUring>, + ring: IoUring, completion_evt: EventFd, } #[derive(Debug)] -pub struct WrappedUserData { +pub struct WrappedRequest { addr: Option, - user_data: T, + req: PendingRequest, } -impl WrappedUserData { - fn new(user_data: T) -> Self { - WrappedUserData { - addr: None, - user_data, - } +impl WrappedRequest { + fn new(req: PendingRequest) -> Self { + WrappedRequest { addr: None, req } } - fn new_with_dirty_tracking(addr: GuestAddress, user_data: T) -> Self { - WrappedUserData { + fn new_with_dirty_tracking(addr: GuestAddress, req: PendingRequest) -> Self { + WrappedRequest { addr: Some(addr), - user_data, + req, } } - fn mark_dirty_mem_and_unwrap(self, mem: &GuestMemoryMmap, count: u32) -> T { + fn mark_dirty_mem_and_unwrap(self, mem: &GuestMemoryMmap, count: u32) -> PendingRequest { if let Some(addr) = self.addr { mem.mark_dirty(addr, count as usize) } - self.user_data + self.req } } -impl AsyncFileEngine { +impl AsyncFileEngine { fn new_ring( file: &File, completion_fd: RawFd, - ) -> Result>, io_uring::IoUringError> { + ) -> Result, IoUringError> { IoUring::new( u32::from(IO_URING_NUM_ENTRIES), vec![file], @@ -90,7 +87,7 @@ impl AsyncFileEngine { ) } - pub fn from_file(file: File) -> Result, AsyncIoError> { + pub fn from_file(file: File) -> Result { log_dev_preview_warning("Async file IO", Option::None); let completion_evt = EventFd::new(libc::EFD_NONBLOCK).map_err(AsyncIoError::EventFd)?; @@ -128,19 +125,19 @@ impl AsyncFileEngine { mem: &GuestMemoryMmap, addr: GuestAddress, count: u32, - user_data: T, - ) -> Result<(), UserDataError> { + req: PendingRequest, + ) -> Result<(), RequestError> { let buf = match mem.get_slice(addr, count as usize) { Ok(slice) => slice.ptr_guard_mut().as_ptr(), Err(err) => { - return Err(UserDataError { - user_data, + return Err(RequestError { + req, error: AsyncIoError::GuestMemory(err), }); } }; - let wrapped_user_data = WrappedUserData::new_with_dirty_tracking(addr, user_data); + let wrapped_user_data = WrappedRequest::new_with_dirty_tracking(addr, req); self.ring .push(Operation::read( @@ -150,8 +147,8 @@ impl AsyncFileEngine { offset, wrapped_user_data, )) - .map_err(|(io_uring_error, data)| UserDataError { - user_data: data.user_data, + .map_err(|(io_uring_error, data)| RequestError { + req: data.req, error: AsyncIoError::IoUring(io_uring_error), }) } @@ -162,19 +159,19 @@ impl AsyncFileEngine { mem: &GuestMemoryMmap, addr: GuestAddress, count: u32, - user_data: T, - ) -> Result<(), UserDataError> { + req: PendingRequest, + ) -> Result<(), RequestError> { let buf = match mem.get_slice(addr, count as usize) { Ok(slice) => slice.ptr_guard_mut().as_ptr(), Err(err) => { - return Err(UserDataError { - user_data, + return Err(RequestError { + req, error: AsyncIoError::GuestMemory(err), }); } }; - let wrapped_user_data = WrappedUserData::new(user_data); + let wrapped_user_data = WrappedRequest::new(req); self.ring .push(Operation::write( @@ -184,19 +181,19 @@ impl AsyncFileEngine { offset, wrapped_user_data, )) - .map_err(|(io_uring_error, data)| UserDataError { - user_data: data.user_data, + .map_err(|(io_uring_error, data)| RequestError { + req: data.req, error: AsyncIoError::IoUring(io_uring_error), }) } - pub fn push_flush(&mut self, user_data: T) -> Result<(), UserDataError> { - let wrapped_user_data = WrappedUserData::new(user_data); + pub fn push_flush(&mut self, req: PendingRequest) -> Result<(), RequestError> { + let wrapped_user_data = WrappedRequest::new(req); self.ring .push(Operation::fsync(0, wrapped_user_data)) - .map_err(|(io_uring_error, data)| UserDataError { - user_data: data.user_data, + .map_err(|(io_uring_error, data)| RequestError { + req: data.req, error: AsyncIoError::IoUring(io_uring_error), }) } @@ -233,11 +230,14 @@ impl AsyncFileEngine { Ok(()) } - fn do_pop(&mut self) -> Result>>, AsyncIoError> { + fn do_pop(&mut self) -> Result>, AsyncIoError> { self.ring.pop().map_err(AsyncIoError::IoUring) } - pub fn pop(&mut self, mem: &GuestMemoryMmap) -> Result>, AsyncIoError> { + pub fn pop( + &mut self, + mem: &GuestMemoryMmap, + ) -> Result>, AsyncIoError> { let cqe = self.do_pop()?.map(|cqe| { let count = cqe.count(); cqe.map_user_data(|wrapped_user_data| { diff --git a/src/vmm/src/devices/virtio/block/virtio/io/mod.rs b/src/vmm/src/devices/virtio/block/virtio/io/mod.rs index 79fe7c0c77d..09cc7c4e31d 100644 --- a/src/vmm/src/devices/virtio/block/virtio/io/mod.rs +++ b/src/vmm/src/devices/virtio/block/virtio/io/mod.rs @@ -9,19 +9,20 @@ use std::fs::File; pub use self::async_io::{AsyncFileEngine, AsyncIoError}; pub use self::sync_io::{SyncFileEngine, SyncIoError}; +use crate::devices::virtio::block::virtio::PendingRequest; use crate::devices::virtio::block::virtio::device::FileEngineType; use crate::vstate::memory::{GuestAddress, GuestMemoryMmap}; -#[derive(Debug, PartialEq, Eq)] -pub struct UserDataOk { - pub user_data: T, +#[derive(Debug)] +pub struct RequestOk { + pub req: PendingRequest, pub count: u32, } -#[derive(Debug, PartialEq, Eq)] -pub enum FileEngineOk { +#[derive(Debug)] +pub enum FileEngineOk { Submitted, - Executed(UserDataOk), + Executed(RequestOk), } #[derive(Debug, thiserror::Error, displaydoc::Display)] @@ -41,25 +42,22 @@ impl BlockIoError { } } -#[derive(Debug, PartialEq, Eq)] -pub struct UserDataError { - pub user_data: T, +#[derive(Debug)] +pub struct RequestError { + pub req: PendingRequest, pub error: E, } #[allow(clippy::large_enum_variant)] #[derive(Debug)] -pub enum FileEngine { +pub enum FileEngine { #[allow(unused)] - Async(AsyncFileEngine), + Async(AsyncFileEngine), Sync(SyncFileEngine), } -impl FileEngine { - pub fn from_file( - file: File, - engine_type: FileEngineType, - ) -> Result, BlockIoError> { +impl FileEngine { + pub fn from_file(file: File, engine_type: FileEngineType) -> Result { match engine_type { FileEngineType::Async => Ok(FileEngine::Async( AsyncFileEngine::from_file(file).map_err(BlockIoError::Async)?, @@ -91,22 +89,20 @@ impl FileEngine { mem: &GuestMemoryMmap, addr: GuestAddress, count: u32, - user_data: T, - ) -> Result, UserDataError> { + req: PendingRequest, + ) -> Result> { match self { - FileEngine::Async(engine) => { - match engine.push_read(offset, mem, addr, count, user_data) { - Ok(_) => Ok(FileEngineOk::Submitted), - Err(err) => Err(UserDataError { - user_data: err.user_data, - error: BlockIoError::Async(err.error), - }), - } - } + FileEngine::Async(engine) => match engine.push_read(offset, mem, addr, count, req) { + Ok(_) => Ok(FileEngineOk::Submitted), + Err(err) => Err(RequestError { + req: err.req, + error: BlockIoError::Async(err.error), + }), + }, FileEngine::Sync(engine) => match engine.read(offset, mem, addr, count) { - Ok(count) => Ok(FileEngineOk::Executed(UserDataOk { user_data, count })), - Err(err) => Err(UserDataError { - user_data, + Ok(count) => Ok(FileEngineOk::Executed(RequestOk { req, count })), + Err(err) => Err(RequestError { + req, error: BlockIoError::Sync(err), }), }, @@ -119,22 +115,20 @@ impl FileEngine { mem: &GuestMemoryMmap, addr: GuestAddress, count: u32, - user_data: T, - ) -> Result, UserDataError> { + req: PendingRequest, + ) -> Result> { match self { - FileEngine::Async(engine) => { - match engine.push_write(offset, mem, addr, count, user_data) { - Ok(_) => Ok(FileEngineOk::Submitted), - Err(err) => Err(UserDataError { - user_data: err.user_data, - error: BlockIoError::Async(err.error), - }), - } - } + FileEngine::Async(engine) => match engine.push_write(offset, mem, addr, count, req) { + Ok(_) => Ok(FileEngineOk::Submitted), + Err(err) => Err(RequestError { + req: err.req, + error: BlockIoError::Async(err.error), + }), + }, FileEngine::Sync(engine) => match engine.write(offset, mem, addr, count) { - Ok(count) => Ok(FileEngineOk::Executed(UserDataOk { user_data, count })), - Err(err) => Err(UserDataError { - user_data, + Ok(count) => Ok(FileEngineOk::Executed(RequestOk { req, count })), + Err(err) => Err(RequestError { + req, error: BlockIoError::Sync(err), }), }, @@ -143,23 +137,20 @@ impl FileEngine { pub fn flush( &mut self, - user_data: T, - ) -> Result, UserDataError> { + req: PendingRequest, + ) -> Result> { match self { - FileEngine::Async(engine) => match engine.push_flush(user_data) { + FileEngine::Async(engine) => match engine.push_flush(req) { Ok(_) => Ok(FileEngineOk::Submitted), - Err(err) => Err(UserDataError { - user_data: err.user_data, + Err(err) => Err(RequestError { + req: err.req, error: BlockIoError::Async(err.error), }), }, FileEngine::Sync(engine) => match engine.flush() { - Ok(_) => Ok(FileEngineOk::Executed(UserDataOk { - user_data, - count: 0, - })), - Err(err) => Err(UserDataError { - user_data, + Ok(_) => Ok(FileEngineOk::Executed(RequestOk { req, count: 0 })), + Err(err) => Err(RequestError { + req, error: BlockIoError::Sync(err), }), }, @@ -204,10 +195,9 @@ pub mod tests { macro_rules! assert_sync_execution { ($expression:expr, $count:expr) => { match $expression { - Ok(FileEngineOk::Executed(UserDataOk { - user_data: _, - count, - })) => assert_eq!(count, $count), + Ok(FileEngineOk::Executed(RequestOk { req: _, count })) => { + assert_eq!(count, $count) + } other => panic!( "Expected: Ok(FileEngineOk::Executed(UserDataOk {{ user_data: _, count: {} \ }})), got: {:?}", @@ -223,7 +213,7 @@ pub mod tests { }; } - fn assert_async_execution(mem: &GuestMemoryMmap, engine: &mut FileEngine<()>, count: u32) { + fn assert_async_execution(mem: &GuestMemoryMmap, engine: &mut FileEngine, count: u32) { if let FileEngine::Async(engine) = engine { engine.drain(false).unwrap(); assert_eq!(engine.pop(mem).unwrap().unwrap().result().unwrap(), count); @@ -271,10 +261,16 @@ pub mod tests { let partial_len = 50; let addr = GuestAddress(MEM_LEN as u64 - u64::from(partial_len)); mem.write(&data, addr).unwrap(); - assert_sync_execution!(engine.write(0, &mem, addr, partial_len, ()), partial_len); + assert_sync_execution!( + engine.write(0, &mem, addr, partial_len, PendingRequest::default()), + partial_len + ); // Partial read let mem = create_mem(); - assert_sync_execution!(engine.read(0, &mem, addr, partial_len, ()), partial_len); + assert_sync_execution!( + engine.read(0, &mem, addr, partial_len, PendingRequest::default()), + partial_len + ); // Check data let mut buf = vec![0u8; partial_len as usize]; mem.read_slice(&mut buf, addr).unwrap(); @@ -286,13 +282,13 @@ pub mod tests { let addr = GuestAddress(0); mem.write(&data, addr).unwrap(); assert_sync_execution!( - engine.write(offset, &mem, addr, partial_len, ()), + engine.write(offset, &mem, addr, partial_len, PendingRequest::default()), partial_len ); // Offset read let mem = create_mem(); assert_sync_execution!( - engine.read(offset, &mem, addr, partial_len, ()), + engine.read(offset, &mem, addr, partial_len, PendingRequest::default()), partial_len ); // Check data @@ -303,13 +299,25 @@ pub mod tests { // Full write mem.write(&data, GuestAddress(0)).unwrap(); assert_sync_execution!( - engine.write(0, &mem, GuestAddress(0), FILE_LEN, ()), + engine.write( + 0, + &mem, + GuestAddress(0), + FILE_LEN, + PendingRequest::default() + ), FILE_LEN ); // Full read let mem = create_mem(); assert_sync_execution!( - engine.read(0, &mem, GuestAddress(0), FILE_LEN, ()), + engine.read( + 0, + &mem, + GuestAddress(0), + FILE_LEN, + PendingRequest::default() + ), FILE_LEN ); // Check data @@ -318,7 +326,7 @@ pub mod tests { assert_eq!(buf, data.as_slice()); // Check other ops - engine.flush(()).unwrap(); + engine.flush(PendingRequest::default()).unwrap(); engine.drain(true).unwrap(); engine.drain_and_flush(true).unwrap(); } @@ -327,7 +335,7 @@ pub mod tests { fn test_async() { // Create backing file. let file = TempFile::new().unwrap().into_file(); - let mut engine = FileEngine::<()>::from_file(file, FileEngineType::Async).unwrap(); + let mut engine = FileEngine::from_file(file, FileEngineType::Async).unwrap(); let data = vmm_sys_util::rand::rand_alphanumerics(FILE_LEN as usize) .as_bytes() @@ -342,11 +350,11 @@ pub mod tests { let partial_len = 50; let addr = GuestAddress(0); mem.write(&data, addr).unwrap(); - assert_queued!(engine.write(offset, &mem, addr, partial_len, ())); + assert_queued!(engine.write(offset, &mem, addr, partial_len, PendingRequest::default())); assert_async_execution(&mem, &mut engine, partial_len); // Offset read let mem = create_mem(); - assert_queued!(engine.read(offset, &mem, addr, partial_len, ())); + assert_queued!(engine.read(offset, &mem, addr, partial_len, PendingRequest::default())); assert_async_execution(&mem, &mut engine, partial_len); // Check data let mut buf = vec![0u8; partial_len as usize]; @@ -358,12 +366,12 @@ pub mod tests { // Full write mem.write(&data, GuestAddress(0)).unwrap(); - assert_queued!(engine.write(0, &mem, addr, FILE_LEN, ())); + assert_queued!(engine.write(0, &mem, addr, FILE_LEN, PendingRequest::default())); assert_async_execution(&mem, &mut engine, FILE_LEN); // Full read let mem = create_mem(); - assert_queued!(engine.read(0, &mem, addr, FILE_LEN, ())); + assert_queued!(engine.read(0, &mem, addr, FILE_LEN, PendingRequest::default())); assert_async_execution(&mem, &mut engine, FILE_LEN); // Check data let mut buf = vec![0u8; FILE_LEN as usize]; @@ -374,7 +382,7 @@ pub mod tests { check_clean_mem(&mem, GuestAddress(4096), 4096); // Check other ops - assert_queued!(engine.flush(())); + assert_queued!(engine.flush(PendingRequest::default())); assert_async_execution(&mem, &mut engine, 0); engine.drain(true).unwrap(); diff --git a/src/vmm/src/devices/virtio/block/virtio/request.rs b/src/vmm/src/devices/virtio/block/virtio/request.rs index f8b3172c5c4..00aba034943 100644 --- a/src/vmm/src/devices/virtio/block/virtio/request.rs +++ b/src/vmm/src/devices/virtio/block/virtio/request.rs @@ -398,13 +398,13 @@ impl Request { match res { Ok(block_io::FileEngineOk::Submitted) => ProcessingResult::Submitted, Ok(block_io::FileEngineOk::Executed(res)) => { - ProcessingResult::Executed(res.user_data.finish(mem, Ok(res.count), block_metrics)) + ProcessingResult::Executed(res.req.finish(mem, Ok(res.count), block_metrics)) } Err(err) => { if err.error.is_throttling_err() { ProcessingResult::Throttled } else { - ProcessingResult::Executed(err.user_data.finish( + ProcessingResult::Executed(err.req.finish( mem, Err(IoErr::FileEngine(err.error)), block_metrics, @@ -426,6 +426,17 @@ mod tests { const NUM_DISK_SECTORS: u64 = 1024; + impl Default for PendingRequest { + fn default() -> Self { + PendingRequest { + r#type: RequestType::In, + data_len: 0, + status_addr: Default::default(), + desc_idx: 0, + } + } + } + #[test] fn test_read_request_header() { let mem = single_region_mem(0x1000);