From bf053ce0f2310ab82e0060c3d15e42d4ddc34e8b Mon Sep 17 00:00:00 2001 From: Ryan Goodfellow Date: Sun, 17 Aug 2025 22:23:53 -0700 Subject: [PATCH 1/4] fix linux p9fs multi message reads --- Cargo.lock | 2 +- Cargo.toml | 2 +- lib/propolis/src/hw/virtio/p9fs.rs | 78 ++++++++++++++++++------------ 3 files changed, 48 insertions(+), 34 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index caa9a5683..49f61f8de 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3905,7 +3905,7 @@ dependencies = [ [[package]] name = "p9ds" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/p9fs#4b8362b894e8ecbb6da34e2e71d7a248918ecd8f" +source = "git+https://github.com/oxidecomputer/p9fs?branch=ry/fix-constructors#cf75ea35fd479f96b68b9ac4cc56b54ad9647203" dependencies = [ "ispf", "num_enum 0.5.11", diff --git a/Cargo.toml b/Cargo.toml index c8f6db530..d7702286b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -71,7 +71,7 @@ propolis-client = { path = "lib/propolis-client" } dlpi = { git = "https://github.com/oxidecomputer/dlpi-sys", branch = "main" } ispf = { git = "https://github.com/oxidecomputer/ispf" } libloading = "0.7" -p9ds = { git = "https://github.com/oxidecomputer/p9fs" } +p9ds = { git = "https://github.com/oxidecomputer/p9fs", branch = "ry/fix-constructors" } softnpu = { git = "https://github.com/oxidecomputer/softnpu" } # Omicron-related diff --git a/lib/propolis/src/hw/virtio/p9fs.rs b/lib/propolis/src/hw/virtio/p9fs.rs index 8cfa8cf65..2dd02986c 100644 --- a/lib/propolis/src/hw/virtio/p9fs.rs +++ b/lib/propolis/src/hw/virtio/p9fs.rs @@ -32,7 +32,6 @@ use p9ds::proto::{ self, Dirent, MessageType, P9Version, Qid, QidType, Rattach, Rclunk, Rgetattr, Rlerror, Rlopen, Rread, Rreaddir, Rstatfs, Rwalk, Tattach, Tgetattr, Tlopen, Tread, Treaddir, Tstatfs, Twalk, Version, - P9_GETATTR_BASIC, }; use slog::{warn, Logger}; @@ -335,50 +334,46 @@ impl HostFSHandler { let file = match fid.file { Some(ref f) => f, None => { - // the file is not open - warn!(self.log, "read: file not open: {:?}", &fid.pathbuf,); return write_error(EINVAL as u32, chain, mem); } }; + let metadata = match file.metadata() { Ok(m) => m, Err(e) => { let ecode = e.raw_os_error().unwrap_or(0); - warn!( - self.log, - "read: metadata for {:?}: {:?}", &fid.pathbuf, e, - ); return write_error(ecode as u32, chain, mem); } }; // bail with empty response if offset is greater than file size if metadata.len() < msg.offset { - warn!( - self.log, - "read: offset > file size: {} > {}", - msg.offset, - metadata.len(), - ); let response = Rread::new(Vec::new()); let mut out = ispf::to_bytes_le(&response).unwrap(); let buf = out.as_mut_slice(); return write_buf(buf, chain, mem); } - let read_count = u32::min(msize, msg.count); - - let space_left = read_count as usize - - size_of::() // Rread.size - - size_of::() // Rread.typ - - size_of::() // Rread.tag - - size_of::() // Rread.data.len - - P9FS_VIRTIO_READ_HEADROOM; - - let buflen = - std::cmp::min(space_left, (metadata.len() - msg.offset) as usize); - - p9_write_file(&file, chain, mem, buflen, msg.offset as i64); + let space_left = u64::from(msize) + - (size_of::() // Rread.size + + size_of::() // Rread.typ + + size_of::() // Rread.tag + + size_of::() // Rread.data.len + + P9FS_VIRTIO_READ_HEADROOM) as u64; + + let msglen = + std::cmp::min(u64::from(msg.count), metadata.len() - msg.offset); + + let buflen = std::cmp::min(space_left, msglen); + + p9_write_file( + &file, + chain, + msg.tag, + mem, + buflen as usize, + msg.offset as i64, + ); } fn do_statfs(&self, fid: &mut Fid, chain: &mut Chain, mem: &MemCtx) { @@ -428,7 +423,14 @@ impl HostFSHandler { write_buf(buf, chain, mem); } - fn do_getattr(&self, fid: &mut Fid, chain: &mut Chain, mem: &MemCtx) { + fn do_getattr( + &self, + fid: &mut Fid, + tag: u16, + valid: u64, + chain: &mut Chain, + mem: &MemCtx, + ) { let metadata = match fs::metadata(&fid.pathbuf) { Ok(m) => m, Err(e) => { @@ -438,7 +440,7 @@ impl HostFSHandler { }; // valid: u64, - let valid = P9_GETATTR_BASIC; + //XXX let valid = P9_GETATTR_BASIC; // qid: Qid, let qid = Qid { typ: { @@ -464,7 +466,15 @@ impl HostFSHandler { // nlink: u64, let nlink = metadata.nlink(); // rdev: u64, - let rdev = metadata.rdev(); + // rdev: u64, + let rdev = + if metadata.is_file() || metadata.is_dir() || metadata.is_symlink() + { + 0 // Regular files, directories, and symlinks should have rdev = 0 + } else { + metadata.rdev() // Only device files should have non-zero rdev + }; + // attrsize: u64, let attrsize = metadata.size(); // blksize: u64, @@ -492,7 +502,7 @@ impl HostFSHandler { // data_version: u64, let data_version = 0; // reserved for future use in spec - let resp = Rgetattr::new( + let mut resp = Rgetattr::new( valid, qid, mode, @@ -514,6 +524,7 @@ impl HostFSHandler { gen, data_version, ); + resp.tag = tag; let mut out = ispf::to_bytes_le(&resp).unwrap(); let buf = out.as_mut_slice(); @@ -915,7 +926,9 @@ impl P9Handler for HostFSHandler { let msg: Tgetattr = ispf::from_bytes_le(&msg_buf).unwrap(); match self.fileserver.lock() { Ok(ref mut fs) => match fs.fids.get_mut(&msg.fid) { - Some(ref mut fid) => self.do_getattr(fid, chain, mem), + Some(ref mut fid) => { + self.do_getattr(fid, msg.tag, msg.request_mask, chain, mem) + } None => { warn!(self.log, "getattr: fid {} not found", msg.fid); return write_error(ENOENT as u32, chain, mem); @@ -954,6 +967,7 @@ pub(crate) fn write_error(ecode: u32, chain: &mut Chain, mem: &MemCtx) { fn p9_write_file( file: &File, chain: &mut Chain, + tag: u16, mem: &MemCtx, count: usize, offset: i64, @@ -976,7 +990,7 @@ fn p9_write_file( let h = Header { size: size as u32, typ: MessageType::Rread as u8, - tag: 0, + tag, count: count as u32, }; From 18f99d3d7baff0bef938295a4e7f194972baaba1 Mon Sep 17 00:00:00 2001 From: Ryan Goodfellow Date: Sun, 17 Aug 2025 23:26:24 -0700 Subject: [PATCH 2/4] bump p9fs --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 49f61f8de..6cdc63a0f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3905,7 +3905,7 @@ dependencies = [ [[package]] name = "p9ds" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/p9fs?branch=ry/fix-constructors#cf75ea35fd479f96b68b9ac4cc56b54ad9647203" +source = "git+https://github.com/oxidecomputer/p9fs#113f170aff6aa1d5add00c19b3a2f3241e16a763" dependencies = [ "ispf", "num_enum 0.5.11", diff --git a/Cargo.toml b/Cargo.toml index d7702286b..c8f6db530 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -71,7 +71,7 @@ propolis-client = { path = "lib/propolis-client" } dlpi = { git = "https://github.com/oxidecomputer/dlpi-sys", branch = "main" } ispf = { git = "https://github.com/oxidecomputer/ispf" } libloading = "0.7" -p9ds = { git = "https://github.com/oxidecomputer/p9fs", branch = "ry/fix-constructors" } +p9ds = { git = "https://github.com/oxidecomputer/p9fs" } softnpu = { git = "https://github.com/oxidecomputer/softnpu" } # Omicron-related From 1847fad6c6b7665c6b6a885272ec7d691ca55862 Mon Sep 17 00:00:00 2001 From: Ryan Goodfellow Date: Mon, 18 Aug 2025 21:27:00 -0700 Subject: [PATCH 3/4] cleanup --- lib/propolis/src/hw/virtio/p9fs.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/propolis/src/hw/virtio/p9fs.rs b/lib/propolis/src/hw/virtio/p9fs.rs index 2dd02986c..6dadd42c1 100644 --- a/lib/propolis/src/hw/virtio/p9fs.rs +++ b/lib/propolis/src/hw/virtio/p9fs.rs @@ -334,6 +334,7 @@ impl HostFSHandler { let file = match fid.file { Some(ref f) => f, None => { + warn!(self.log, "read: file not open: {:?}", &fid.pathbuf,); return write_error(EINVAL as u32, chain, mem); } }; @@ -342,12 +343,22 @@ impl HostFSHandler { Ok(m) => m, Err(e) => { let ecode = e.raw_os_error().unwrap_or(0); + warn!( + self.log, + "read: metadata for {:?}: {:?}", &fid.pathbuf, e, + ); return write_error(ecode as u32, chain, mem); } }; // bail with empty response if offset is greater than file size if metadata.len() < msg.offset { + warn!( + self.log, + "read: offset > file size: {} > {}", + msg.offset, + metadata.len(), + ); let response = Rread::new(Vec::new()); let mut out = ispf::to_bytes_le(&response).unwrap(); let buf = out.as_mut_slice(); @@ -439,8 +450,6 @@ impl HostFSHandler { } }; - // valid: u64, - //XXX let valid = P9_GETATTR_BASIC; // qid: Qid, let qid = Qid { typ: { @@ -458,7 +467,6 @@ impl HostFSHandler { // mode: u32, let mode = metadata.mode(); // uid: u32, - //let uid = metadata.uid(); let uid = 0; //squash for now // gid: u32, //let gid = metadata.gid(); @@ -466,7 +474,6 @@ impl HostFSHandler { // nlink: u64, let nlink = metadata.nlink(); // rdev: u64, - // rdev: u64, let rdev = if metadata.is_file() || metadata.is_dir() || metadata.is_symlink() { From 4cf344073389442d82cdcb31471fb08b7eec4bda Mon Sep 17 00:00:00 2001 From: Ryan Goodfellow Date: Tue, 19 Aug 2025 16:39:29 -0700 Subject: [PATCH 4/4] review feedback --- lib/propolis/src/hw/virtio/p9fs.rs | 41 +++++++++++++----------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/lib/propolis/src/hw/virtio/p9fs.rs b/lib/propolis/src/hw/virtio/p9fs.rs index 6dadd42c1..4f6ab0366 100644 --- a/lib/propolis/src/hw/virtio/p9fs.rs +++ b/lib/propolis/src/hw/virtio/p9fs.rs @@ -43,6 +43,19 @@ use slog::{warn, Logger}; /// RREAD header stated size. const P9FS_VIRTIO_READ_HEADROOM: usize = 20; +// 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. +#[repr(C, packed)] +#[derive(Copy, Clone)] +struct RreadHeader { + size: u32, + typ: u8, + tag: u16, + count: u32, +} + #[usdt::provider(provider = "propolis")] mod probes { fn p9fs_cfg_read() {} @@ -366,18 +379,14 @@ impl HostFSHandler { } let space_left = u64::from(msize) - - (size_of::() // Rread.size - + size_of::() // Rread.typ - + size_of::() // Rread.tag - + size_of::() // Rread.data.len - + P9FS_VIRTIO_READ_HEADROOM) as u64; + - (size_of::() + P9FS_VIRTIO_READ_HEADROOM) as u64; let msglen = std::cmp::min(u64::from(msg.count), metadata.len() - msg.offset); let buflen = std::cmp::min(space_left, msglen); - p9_write_file( + p9_read_file( &file, chain, msg.tag, @@ -469,7 +478,6 @@ impl HostFSHandler { // uid: u32, let uid = 0; //squash for now // gid: u32, - //let gid = metadata.gid(); let gid = 0; //squash for now // nlink: u64, let nlink = metadata.nlink(); @@ -971,7 +979,7 @@ pub(crate) fn write_error(ecode: u32, chain: &mut Chain, mem: &MemCtx) { write_buf(buf, chain, mem); } -fn p9_write_file( +fn p9_read_file( file: &File, chain: &mut Chain, tag: u16, @@ -979,22 +987,9 @@ fn p9_write_file( 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. - #[repr(C, packed)] - #[derive(Copy, Clone)] - struct Header { - size: u32, - typ: u8, - tag: u16, - count: u32, - } - - let size = size_of::
() + count; + let size = size_of::() + count; - let h = Header { + let h = RreadHeader { size: size as u32, typ: MessageType::Rread as u8, tag,