Refactor FUSE #1047
Refactor FUSE #1047
Conversation
cagatay-y
force-pushed
the
fuse-refactor-2
branch
from
69b0447
to
be7636e
Compare
|
Some of the commits that were automatically rebased do not build. I thought it may make more sense to get feedback first and fix the commits later, incorporating the feedback. I also could not figure out why Clippy thinks that ROOT_ID is not used. |
cagatay-y
force-pushed
the
fuse-refactor-2
branch
from
0899de4
to
601846e
Compare
There was a problem hiding this comment.
Thanks a lot, @cagatay-y!
This is looking great, though I have a few requests. Please don't hesitate to reach out, if you want to talk about them.
src/drivers/fs/virtio_fs.rs
Outdated
| let transfer_tkn = buff_tkn | ||
| .write(Some(cmd), None::<&<fuse::Op<CODE> as fuse::OpTrait>::Rsp>) | ||
| .unwrap(); |
There was a problem hiding this comment.
Did you change Some(rsp) to None purposefully?
There was a problem hiding this comment.
Yes. As far as I understand, FUSE does not need the memory allocated for the response to be initialized at all. It reads the size of the writable section from the command struct. BufferToken::write copies the the argument when it is Some and since we don't initialize the response, I bypassed the copying.
There was a problem hiding this comment.
The part of the request structure that corresponds to our Rsp is referred to as a “[d]evice-writable part” in the file system device section of the Virtio specification and the Virtqueue section specifies that “a device SHOULD NOT read a device-writable buffer (it MAY do so for debugging or diagnostic purposes).”
There was a problem hiding this comment.
Since this is true for Virtqueue's in general, I think we can also remove receive buffer copying from the Virtqueue interface (unless we want to keep it for “debugging or diagnostic purposes,” as mentioned in the specification), though it should probably be a separate PR since it touches parts of the code base outside FUSE.
src/fs/fuse.rs
Outdated
| pub flags: u32, | ||
| pub padding: u32, | ||
| impl Op<{ fuse_abi::Opcode::Init as u32 }> { | ||
| fn create() -> (Box<<Self as OpTrait>::Cmd>, Box<<Self as OpTrait>::Rsp>) { |
There was a problem hiding this comment.
For Sized return types, we might want to avoid boxing if possible. Though that might be better off as a separate PR, since boxing was the previous behavior too, I think.
cagatay-y
force-pushed
the
fuse-refactor-2
branch
4 times, most recently
from
43de9b3
to
fba4e32
Compare
Moving the ABI definitions to a separate module helps keeping track of what is our design and what is dictated by the FUSE ABI.
Since the convention removes the module prefix from the identifiers, qualified names are used at use-sites.
The host does not use the len field of the header of the Rsp (relies on the size field in the In struct if necessary instead). It is not necessary to fill it in during creation and copy it during dispatch.
Turning the Cmd/Rsp creation functions into associated functions for the Op structs allows us to refer to the Op struct in the function signature and body in a more concise manner.
To specify operations, use structs instead of const generic arguments to reduce verbosity.
cagatay-y
force-pushed
the
fuse-refactor-2
branch
from
ce25dfe
to
bd900cd
Compare
This PR aims to use safer standard library functionality for allocation for FUSE, deduplicate what is in common between different operations and specify what is necessarily varying between them in a easier-to-read manner.
Another goal is to set up the module so that in the future we can access the
Rsplayout without creating a skeletonRspobject, in order to eliminate copying. However, that requires changes in virtqueue API and is not a part of this PR.