From af15e5c546aded0ca8de3a9da4cafebf9ba3412f Mon Sep 17 00:00:00 2001 From: Ryan Goodfellow Date: Mon, 27 Nov 2023 15:05:44 -0800 Subject: [PATCH 1/4] implement p9fs rread as direct host file to guest mem write Comes with ~5x performance improvement. The following is on a 2.17G transfer. * Before real 2:14.557405302 user 5.665261225 sys 15.781882665 * After real 23.913634189 user 6.123950329 sys 15.342021778 --- lib/propolis/src/hw/virtio/p9fs.rs | 32 +++--------------- lib/propolis/src/hw/virtio/queue.rs | 52 +++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 28 deletions(-) diff --git a/lib/propolis/src/hw/virtio/p9fs.rs b/lib/propolis/src/hw/virtio/p9fs.rs index 9662c962a..425cbd8eb 100644 --- a/lib/propolis/src/hw/virtio/p9fs.rs +++ b/lib/propolis/src/hw/virtio/p9fs.rs @@ -5,9 +5,9 @@ use std::collections::HashMap; use std::convert::TryInto; use std::fs; -use std::io::{Read, Seek}; use std::mem::size_of; use std::num::NonZeroU16; +use std::os::fd::AsRawFd; use std::os::unix::ffi::OsStrExt; use std::os::unix::fs::MetadataExt; use std::path::PathBuf; @@ -21,7 +21,7 @@ use crate::vmm::MemCtx; use super::bits::*; use super::pci::{PciVirtio, PciVirtioState}; -use super::queue::{write_buf, Chain, VirtQueue, VirtQueues}; +use super::queue::{p9_write_file, write_buf, Chain, VirtQueue, VirtQueues}; use super::VirtioDevice; use ispf::WireSize; @@ -332,7 +332,7 @@ impl HostFSHandler { mem: &MemCtx, msize: u32, ) { - let mut file = match fid.file { + let file = match fid.file { Some(ref f) => f, None => { // the file is not open @@ -366,15 +366,6 @@ impl HostFSHandler { return write_buf(buf, chain, mem); } - match file.seek(std::io::SeekFrom::Start(msg.offset)) { - Err(e) => { - let ecode = e.raw_os_error().unwrap_or(0); - warn!(self.log, "read: seek: {:?}: {:?}", &fid.pathbuf, e,); - return write_error(ecode as u32, chain, mem); - } - Ok(_) => {} - } - let read_count = u32::min(msize, msg.count); let space_left = read_count as usize @@ -387,22 +378,7 @@ impl HostFSHandler { let buflen = std::cmp::min(space_left, (metadata.len() - msg.offset) as usize); - let mut content: Vec = vec![0; buflen]; - - match file.read_exact(content.as_mut_slice()) { - Err(e) => { - let ecode = e.raw_os_error().unwrap_or(0); - warn!(self.log, "read: exact: {:?}: {:?}", &fid.pathbuf, e,); - return write_error(ecode as u32, chain, mem); - } - Ok(()) => {} - } - - let response = Rread::new(content); - let mut out = ispf::to_bytes_le(&response).unwrap(); - let buf = out.as_mut_slice(); - - write_buf(buf, chain, mem); + p9_write_file(&file.as_raw_fd(), chain, mem, buflen, msg.offset as i64); } fn do_statfs(&self, fid: &mut Fid, chain: &mut Chain, mem: &MemCtx) { diff --git a/lib/propolis/src/hw/virtio/queue.rs b/lib/propolis/src/hw/virtio/queue.rs index d520ae4bc..d2456b467 100644 --- a/lib/propolis/src/hw/virtio/queue.rs +++ b/lib/propolis/src/hw/virtio/queue.rs @@ -770,3 +770,55 @@ pub(crate) fn write_buf(buf: &[u8], chain: &mut Chain, mem: &MemCtx) { } }); } + +#[cfg(feature = "falcon")] +pub(crate) fn p9_write_file( + file: &impl std::os::fd::AsRawFd, + chain: &mut Chain, + mem: &MemCtx, + count: usize, + offset: i64, +) { + // Form the rread header. Unfortunately we can't do this with the Rread + // structure because the count is baked into the data field which is tied + // to the length of the vector and filling that vector is what we're + // explicitly trying to avoid here. + let sz = mem::size_of::() + // size + mem::size_of::() + // typ + mem::size_of::() + // tag + mem::size_of::() + // data.count + count; // data + let mut header = Vec::with_capacity(11); + header.extend_from_slice(&(sz as u32).to_le_bytes()); + header.push(p9ds::proto::MessageType::Rread as u8); + header.extend_from_slice(&0u16.to_le_bytes()); + header.extend_from_slice(&(count as u32).to_le_bytes()); + + // Send the header to the guest from the buffer constructed above. Then + // send the actual file data + let mut header_done = false; + let mut done = 0; + let _total = chain.for_remaining_type(false, |addr, len| { + let mut remain = len; + let mut copied = 0; + if !header_done { + mem.write_from(addr, &header, header.len()).unwrap(); + header_done = true; + remain -= header.len(); + copied += header.len(); + } + let addr = GuestAddr(addr.0 + copied as u64); + let sub_mapping = + mem.direct_writable_region(&GuestRegion(addr, remain)).unwrap(); + + let len = usize::min(remain, count); + let off = offset + done as i64; + let mapped = sub_mapping.pread(file, len, off).unwrap(); + copied += mapped; + done += mapped; + + let need_more = done < count; + + (copied, need_more) + }); +} From 06ffb39b3fc083fc3dc9fa4d84d139277f049421 Mon Sep 17 00:00:00 2001 From: Ryan Goodfellow Date: Mon, 27 Nov 2023 15:40:21 -0800 Subject: [PATCH 2/4] move p9_write_file to p9fs.rs --- lib/propolis/src/hw/virtio/p9fs.rs | 53 ++++++++++++++++++++++++++++- lib/propolis/src/hw/virtio/queue.rs | 53 ----------------------------- 2 files changed, 52 insertions(+), 54 deletions(-) diff --git a/lib/propolis/src/hw/virtio/p9fs.rs b/lib/propolis/src/hw/virtio/p9fs.rs index 425cbd8eb..6b22e1653 100644 --- a/lib/propolis/src/hw/virtio/p9fs.rs +++ b/lib/propolis/src/hw/virtio/p9fs.rs @@ -21,7 +21,7 @@ use crate::vmm::MemCtx; use super::bits::*; use super::pci::{PciVirtio, PciVirtioState}; -use super::queue::{p9_write_file, write_buf, Chain, VirtQueue, VirtQueues}; +use super::queue::{write_buf, Chain, VirtQueue, VirtQueues}; use super::VirtioDevice; use ispf::WireSize; @@ -950,3 +950,54 @@ pub(crate) fn write_error(ecode: u32, chain: &mut Chain, mem: &MemCtx) { let buf = out.as_mut_slice(); write_buf(buf, chain, mem); } + +pub(crate) fn p9_write_file( + file: &impl std::os::fd::AsRawFd, + chain: &mut Chain, + mem: &MemCtx, + count: usize, + offset: i64, +) { + // Form the rread header. Unfortunately we can't do this with the Rread + // structure because the count is baked into the data field which is tied + // to the length of the vector and filling that vector is what we're + // explicitly trying to avoid here. + let sz = size_of::() + // size + size_of::() + // typ + size_of::() + // tag + size_of::() + // data.count + count; // data + let mut header = Vec::with_capacity(11); + header.extend_from_slice(&(sz as u32).to_le_bytes()); + header.push(p9ds::proto::MessageType::Rread as u8); + header.extend_from_slice(&0u16.to_le_bytes()); + header.extend_from_slice(&(count as u32).to_le_bytes()); + + // Send the header to the guest from the buffer constructed above. Then + // send the actual file data + let mut header_done = false; + let mut done = 0; + let _total = chain.for_remaining_type(false, |addr, len| { + let mut remain = len; + let mut copied = 0; + if !header_done { + mem.write_from(addr, &header, header.len()).unwrap(); + header_done = true; + remain -= header.len(); + copied += header.len(); + } + let addr = GuestAddr(addr.0 + copied as u64); + let sub_mapping = + mem.direct_writable_region(&GuestRegion(addr, remain)).unwrap(); + + let len = usize::min(remain, count); + let off = offset + done as i64; + let mapped = sub_mapping.pread(file, len, off).unwrap(); + copied += mapped; + done += mapped; + + let need_more = done < count; + + (copied, need_more) + }); +} diff --git a/lib/propolis/src/hw/virtio/queue.rs b/lib/propolis/src/hw/virtio/queue.rs index d2456b467..0db1a6d6e 100644 --- a/lib/propolis/src/hw/virtio/queue.rs +++ b/lib/propolis/src/hw/virtio/queue.rs @@ -752,7 +752,6 @@ pub mod migrate { } } -#[cfg(feature = "falcon")] pub(crate) fn write_buf(buf: &[u8], chain: &mut Chain, mem: &MemCtx) { // more copy pasta from Chain::write b/c like Chain:read a // statically sized type is expected. @@ -770,55 +769,3 @@ pub(crate) fn write_buf(buf: &[u8], chain: &mut Chain, mem: &MemCtx) { } }); } - -#[cfg(feature = "falcon")] -pub(crate) fn p9_write_file( - file: &impl std::os::fd::AsRawFd, - chain: &mut Chain, - mem: &MemCtx, - count: usize, - offset: i64, -) { - // Form the rread header. Unfortunately we can't do this with the Rread - // structure because the count is baked into the data field which is tied - // to the length of the vector and filling that vector is what we're - // explicitly trying to avoid here. - let sz = mem::size_of::() + // size - mem::size_of::() + // typ - mem::size_of::() + // tag - mem::size_of::() + // data.count - count; // data - let mut header = Vec::with_capacity(11); - header.extend_from_slice(&(sz as u32).to_le_bytes()); - header.push(p9ds::proto::MessageType::Rread as u8); - header.extend_from_slice(&0u16.to_le_bytes()); - header.extend_from_slice(&(count as u32).to_le_bytes()); - - // Send the header to the guest from the buffer constructed above. Then - // send the actual file data - let mut header_done = false; - let mut done = 0; - let _total = chain.for_remaining_type(false, |addr, len| { - let mut remain = len; - let mut copied = 0; - if !header_done { - mem.write_from(addr, &header, header.len()).unwrap(); - header_done = true; - remain -= header.len(); - copied += header.len(); - } - let addr = GuestAddr(addr.0 + copied as u64); - let sub_mapping = - mem.direct_writable_region(&GuestRegion(addr, remain)).unwrap(); - - let len = usize::min(remain, count); - let off = offset + done as i64; - let mapped = sub_mapping.pread(file, len, off).unwrap(); - copied += mapped; - done += mapped; - - let need_more = done < count; - - (copied, need_more) - }); -} From 97b74e2b616186d8446c8955474863c03b1af274 Mon Sep 17 00:00:00 2001 From: Ryan Goodfellow Date: Mon, 27 Nov 2023 16:52:21 -0800 Subject: [PATCH 3/4] review feedback --- lib/propolis/src/hw/virtio/p9fs.rs | 53 ++++++++++++++--------------- lib/propolis/src/hw/virtio/queue.rs | 1 + 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/lib/propolis/src/hw/virtio/p9fs.rs b/lib/propolis/src/hw/virtio/p9fs.rs index 6b22e1653..08cc50f94 100644 --- a/lib/propolis/src/hw/virtio/p9fs.rs +++ b/lib/propolis/src/hw/virtio/p9fs.rs @@ -4,10 +4,9 @@ use std::collections::HashMap; use std::convert::TryInto; -use std::fs; +use std::fs::{self, File}; use std::mem::size_of; use std::num::NonZeroU16; -use std::os::fd::AsRawFd; use std::os::unix::ffi::OsStrExt; use std::os::unix::fs::MetadataExt; use std::path::PathBuf; @@ -378,7 +377,7 @@ impl HostFSHandler { let buflen = std::cmp::min(space_left, (metadata.len() - msg.offset) as usize); - p9_write_file(&file.as_raw_fd(), chain, mem, buflen, msg.offset as i64); + p9_write_file(&file, chain, mem, buflen, msg.offset as i64); } fn do_statfs(&self, fid: &mut Fid, chain: &mut Chain, mem: &MemCtx) { @@ -951,8 +950,8 @@ pub(crate) fn write_error(ecode: u32, chain: &mut Chain, mem: &MemCtx) { write_buf(buf, chain, mem); } -pub(crate) fn p9_write_file( - file: &impl std::os::fd::AsRawFd, +fn p9_write_file( + file: &File, chain: &mut Chain, mem: &MemCtx, count: usize, @@ -962,35 +961,35 @@ pub(crate) fn p9_write_file( // structure because the count is baked into the data field which is tied // to the length of the vector and filling that vector is what we're // explicitly trying to avoid here. - let sz = size_of::() + // size - size_of::() + // typ - size_of::() + // tag - size_of::() + // data.count - count; // data - let mut header = Vec::with_capacity(11); - header.extend_from_slice(&(sz as u32).to_le_bytes()); - header.push(p9ds::proto::MessageType::Rread as u8); - header.extend_from_slice(&0u16.to_le_bytes()); - header.extend_from_slice(&(count as u32).to_le_bytes()); + #[repr(C, packed)] + #[derive(Copy, Clone)] + struct Header { + size: u32, + typ: u8, + tag: u16, + count: u32, + } + + let size = size_of::
() + count; + + let h = Header { + size: size as u32, + typ: MessageType::Rread as u8, + tag: 0, + count: count as u32, + }; + + chain.write(&h, mem); // Send the header to the guest from the buffer constructed above. Then // send the actual file data - let mut header_done = false; let mut done = 0; let _total = chain.for_remaining_type(false, |addr, len| { - let mut remain = len; let mut copied = 0; - if !header_done { - mem.write_from(addr, &header, header.len()).unwrap(); - header_done = true; - remain -= header.len(); - copied += header.len(); - } - let addr = GuestAddr(addr.0 + copied as u64); - let sub_mapping = - mem.direct_writable_region(&GuestRegion(addr, remain)).unwrap(); + let addr = addr.offset::(copied); + let sub_mapping = mem.writable_region(&GuestRegion(addr, len)).unwrap(); - let len = usize::min(remain, count); + let len = usize::min(len, count); let off = offset + done as i64; let mapped = sub_mapping.pread(file, len, off).unwrap(); copied += mapped; diff --git a/lib/propolis/src/hw/virtio/queue.rs b/lib/propolis/src/hw/virtio/queue.rs index 0db1a6d6e..d520ae4bc 100644 --- a/lib/propolis/src/hw/virtio/queue.rs +++ b/lib/propolis/src/hw/virtio/queue.rs @@ -752,6 +752,7 @@ pub mod migrate { } } +#[cfg(feature = "falcon")] pub(crate) fn write_buf(buf: &[u8], chain: &mut Chain, mem: &MemCtx) { // more copy pasta from Chain::write b/c like Chain:read a // statically sized type is expected. From 8633ba6b6cfce274f6a7b957f63a488644923492 Mon Sep 17 00:00:00 2001 From: Ryan Goodfellow Date: Tue, 28 Nov 2023 13:39:57 -0800 Subject: [PATCH 4/4] review feedback --- lib/propolis/src/hw/virtio/p9fs.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/propolis/src/hw/virtio/p9fs.rs b/lib/propolis/src/hw/virtio/p9fs.rs index 08cc50f94..886bcd924 100644 --- a/lib/propolis/src/hw/virtio/p9fs.rs +++ b/lib/propolis/src/hw/virtio/p9fs.rs @@ -985,18 +985,13 @@ fn p9_write_file( // send the actual file data let mut done = 0; let _total = chain.for_remaining_type(false, |addr, len| { - let mut copied = 0; - let addr = addr.offset::(copied); let sub_mapping = mem.writable_region(&GuestRegion(addr, len)).unwrap(); - let len = usize::min(len, count); let off = offset + done as i64; let mapped = sub_mapping.pread(file, len, off).unwrap(); - copied += mapped; done += mapped; let need_more = done < count; - - (copied, need_more) + (mapped, need_more) }); }