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

Conversation

rcgoodfellow
Copy link
Contributor

@rcgoodfellow rcgoodfellow commented Aug 18, 2025

  • Fix space left calculation to be based on maximum message size rather than client requested size.
  • Fix tag propagation for some p9fs message types.
  • Bump p9fs library that has message type tag fixes.
  • Tie Rgetattr.valid to Tgetattr.mask

@rcgoodfellow rcgoodfellow marked this pull request as ready for review August 19, 2025 04:35
@taspelund
Copy link

The code changes seem sensible to me, but I don't have a lot of context around p9fs or filesystem implementations in general, so I don't know whether this is the right thing to do semantically

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)
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

@pfmooney pfmooney self-requested a review August 19, 2025 21:20
Copy link
Member

@iximeow iximeow left a comment

Choose a reason for hiding this comment

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

notionally 👍 but a smattering of nits to offer. Is this one of those things we Ought To Test Better but we just haven't done w/ phd or something?

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)
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?

@rcgoodfellow
Copy link
Contributor Author

rcgoodfellow commented Aug 19, 2025

we Ought To Test Better but we just haven't done w/ phd or something?

The p9fs stuff is behind the falcon feature and is use exclusively in falcon for testbeds. It's not compiled into the production binary. So it does not have the same level of testing as product features. Tbh most of it has been done as quickly as possible by me in service of creating a testing surface for urgent product features.

@iximeow
Copy link
Member

iximeow commented Aug 20, 2025

use exclusively in falcon for testbeds

ah i'd forgotten it was fully sequestered like that, fair enough

Copy link
Contributor

@dancrossnyc dancrossnyc left a comment

Choose a reason for hiding this comment

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

LG! Thanks, Ry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants