Inflight I/O: Implement missing traits #20

harshanavkis · 2021-07-21T11:40:38Z

This commit implements the get and set inflight fd members of the
VhostUserSlaveReqHandlerMut trait, which is used to pass inflight
I/O queue tracking regions as memfds.

Fixes #14.

Signed-off-by: Harshavardhan Unnibhavi harshanavkis@gmail.com

Cargo.toml

harshanavkis · 2021-07-21T15:39:32Z

@sboeuf I changed the nix calls to libc calls.

sboeuf

You need to think about a way to share the inflight file with the consumer of the vhost_user_backend crate so that it can fill the inflight region.
I think the point is to handle as much as we can from this crate (which means receiving the fd and creating the mapping), and let the consumers deal with the content of what they wanna write into this region.

sboeuf · 2021-07-22T12:22:30Z

src/lib.rs

+                libc::MFD_CLOEXEC,
+            )
+        };
+


Did you use vu_get_inflight_fd() from https://github.com/qemu/qemu/blob/master/subprojects/libvhost-user/libvhost-user.c#L1654 as a reference?
From what I see, you forgot to call into fcntl(*fd, F_ADD_SEALS, flags) in order to add F_SEAL_GROW, F_SEAL_SHRINK and F_SEAL_SEAL to setup the proper seals, and you didn't mmap() the fd into the vhost-user process address space.

sboeuf · 2021-07-22T12:29:09Z

src/lib.rs

+        _inflight: &VhostUserInflight,
+        file: File,
+    ) -> VhostUserResult<()> {
+        self.inflight_file = Some(file);


Same here, please use https://github.com/qemu/qemu/blob/master/subprojects/libvhost-user/libvhost-user.c#L1706 as a reference, and make sure to update the file after you called into mmap().
The previous inflight_file will be closed automatically by Rust since you're replacing it with the new file you just received, so we're good there. But you must also unmap what has been previously mapped through get_inflight_fd(). That's why you need to store something like inflight_mapping_addr: Option<GuestAddress> to be able to know if a previous mapping happened.

harshanavkis · 2021-07-26T14:31:48Z

@sboeuf I have reimplemented the get and set functionalities. The current error handling in functions like "memfd_alloc" compares return values and converts it into the corresponding pointer. Would it be better to create a new error variant in vhost_user::Error, to handle each such case?

sboeuf · 2021-07-26T14:53:27Z

@jiangliu

Would it be better to create a new error variant in vhost_user::Error, to handle each such case?

What do you think?

jiangliu · 2021-09-09T09:49:12Z

@jiangliu

Would it be better to create a new error variant in vhost_user::Error, to handle each such case?

What do you think?

Great, we should do that:)

jiangliu · 2021-09-09T09:50:45Z

@harshanavkis Could you please help to rebase this MR so we could include it into v0.2.0?

harshanavkis · 2021-09-09T14:04:23Z

@jiangliu sure, will get this done.

harshanavkis · 2021-09-09T23:44:28Z

I have created a pull request here: rust-vmm/vhost#84, to add the error enums for handling errors during get_inflight_fd.

Cargo.toml

src/handler.rs

src/lib.rs

src/handler.rs

jiangliu · 2021-12-22T15:42:35Z

src/handler.rs

+            return Err(VhostUserError::MemFdCreateError);
+        }
+
+        inflight_file = ret_val;


inflight_file is a RawFd, so it need to be manually closed on error handling path. Otherwise it will get leaked.

The inflight_file is actually a RawFd, and it's error prone to deal with RawFd.
So it would be better to convert the infight_file from RawFd into a File object, which eases the error handling cases.

jiangliu · 2021-12-22T15:43:59Z

src/handler.rs

+        // map the inflight_file into memory.
+        // unsafe as extern mmap is called
+        Ok((
+            unsafe {


We need to handle the case that mmap fails.

I see, makes sense. Perhaps we should add a new error enum into vhost_user::Error?

It could be solved/refined by refactor the code. The suggestion is to avoid RawFd if possible, because it doesn't solve the file descriptor ownership. The way to avoid RawFd is to convert it into File as soon as possible. And it's safe and easy to get RawFd from File object by file.as_raw_fd().

src/handler.rs

jiangliu · 2021-12-22T15:58:49Z

src/handler.rs

+    }
+
+    fn set_desc_num_split(&mut self, inflight_region: u64, num_queues: u16, queue_size: u16) {
+        let raw_ptr = inflight_region as *mut QueueRegionSplit;


How about this for code readability:

let mut regions = unsafe { slice::from_raw_parts_mut(inflight_region as *mut QueueRegionSplit, num_queues as usize) }; regions.iter_mut().map(|v| v.desc_num = queue_size);

Rust complains about unsused Map because of lazy iterators so I changed it slightly, let me know what you think.

jiangliu · 2021-12-22T16:03:50Z

src/handler.rs

+            self.get_inflight_queue_size(inflight.queue_size) * inflight.num_queues as usize;
+
+        // Create a memfd region to hold the queues for inflight I/O tracking
+        let (mmap_ptr, inflight_fd, dup_inflight_fd) = self.memfd_alloc(total_mmap_size)?;


Seems it will get simpler if moves following "sef.inflight_state.set_inflight_state()" into memfd_alloc()

This commit implements the get and set inflight fd members of the VhostUserSlaveReqHandlerMut trait, which is used to pass inflight I/O queue tracking regions as memfds. Fixes rust-vmm#14. Signed-off-by: Harshavardhan Unnibhavi <harshanavkis@gmail.com>

Signed-off-by: Harshavardhan Unnibhavi <harshanavkis@gmail.com>

harshanavkis force-pushed the inflight-traits branch 2 times, most recently from 9b9ee56 to 347357a Compare July 21, 2021 12:03

harshanavkis marked this pull request as ready for review July 21, 2021 12:25

harshanavkis requested review from jiangliu, sboeuf and slp as code owners July 21, 2021 12:25

sboeuf reviewed Jul 21, 2021

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

harshanavkis force-pushed the inflight-traits branch 3 times, most recently from fd1e8e5 to 0e0c52e Compare July 21, 2021 15:33

sboeuf suggested changes Jul 22, 2021

View reviewed changes

harshanavkis force-pushed the inflight-traits branch 5 times, most recently from ab2297c to b99b485 Compare July 26, 2021 13:55

harshanavkis force-pushed the inflight-traits branch from b99b485 to b33b3bd Compare September 9, 2021 21:57

harshanavkis requested a review from stefano-garzarella as a code owner September 9, 2021 21:57

harshanavkis force-pushed the inflight-traits branch 2 times, most recently from 6a3e7ce to 2ccceca Compare September 9, 2021 23:38

stefano-garzarella suggested changes Sep 10, 2021

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

harshanavkis force-pushed the inflight-traits branch from 2ccceca to 6393439 Compare September 10, 2021 08:42

stefano-garzarella reviewed Sep 10, 2021

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

harshanavkis force-pushed the inflight-traits branch 10 times, most recently from 7d84393 to 8faef7d Compare December 22, 2021 11:54