Skip to content

fix linux p9fs multi message reads #932

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

90 changes: 53 additions & 37 deletions lib/propolis/src/hw/virtio/p9fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand All @@ -44,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() {}
Expand Down Expand Up @@ -335,11 +347,11 @@ 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) => {
Expand All @@ -366,19 +378,22 @@ impl HostFSHandler {
return write_buf(buf, chain, mem);
}

let read_count = u32::min(msize, msg.count);
let space_left = u64::from(msize)
Copy link
Contributor Author

@rcgoodfellow rcgoodfellow Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The space left for a p9fs message needs to be computed in terms of the maximum message size (msize in p9fs parlance), and not the read count from the client.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taking this as an excuse to learn about p9fs, this is more like MAX_READ_SIZE now right? e.g. given the current msize, a read can safely be satisfied with up to this many bytes.

then the issue before was that if a client asked to read, say, 20 bytes, we'd calculate we could only return 9?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's correct

- (size_of::<RreadHeader>() + P9FS_VIRTIO_READ_HEADROOM) as u64;

let space_left = read_count as usize
- size_of::<u32>() // Rread.size
- size_of::<MessageType>() // Rread.typ
- size_of::<u16>() // Rread.tag
- size_of::<u32>() // Rread.data.len
- P9FS_VIRTIO_READ_HEADROOM;
let msglen =
std::cmp::min(u64::from(msg.count), metadata.len() - msg.offset);

let buflen =
std::cmp::min(space_left, (metadata.len() - msg.offset) as usize);
let buflen = std::cmp::min(space_left, msglen);

p9_write_file(&file, chain, mem, buflen, msg.offset as i64);
p9_read_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) {
Expand Down Expand Up @@ -428,7 +443,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) => {
Expand All @@ -437,8 +459,6 @@ impl HostFSHandler {
}
};

// valid: u64,
let valid = P9_GETATTR_BASIC;
// qid: Qid,
let qid = Qid {
typ: {
Expand All @@ -456,15 +476,20 @@ 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();
let gid = 0; //squash for now
// nlink: u64,
let nlink = metadata.nlink();
// rdev: u64,
let rdev = metadata.rdev();
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,
Expand Down Expand Up @@ -492,7 +517,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,
Expand All @@ -514,6 +539,7 @@ impl HostFSHandler {
gen,
data_version,
);
resp.tag = tag;

let mut out = ispf::to_bytes_le(&resp).unwrap();
let buf = out.as_mut_slice();
Expand Down Expand Up @@ -915,7 +941,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);
Expand Down Expand Up @@ -951,32 +979,20 @@ 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,
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.
#[repr(C, packed)]
#[derive(Copy, Clone)]
struct Header {
size: u32,
typ: u8,
tag: u16,
count: u32,
}

let size = size_of::<Header>() + count;
let size = size_of::<RreadHeader>() + count;

let h = Header {
let h = RreadHeader {
size: size as u32,
typ: MessageType::Rread as u8,
tag: 0,
tag,
count: count as u32,
};

Expand Down
Loading