Another iteration of supporting file backed memory regions #29

alexandruag · 2019-06-11T08:18:14Z

This PR is based on code from Sebastien's original proposal (#20) and the subsequent comments and conversations.

As per Paolo's suggestion, the offset is now represented as an u64. I didn't name the new struct FileRange, because it should've also kept a length field, and that information is already available via the len() method of the GuestMemoryRegion. Instead I've called it FileOffset (will rename on demand), and it only keeps the Arc<File> and offset.

The Linux code is clearly ahead of the Windows one right now, and some harmonization is in order. I've just created a Windows VM on my machine, but didn't get the chance to install Rust and test things yet. It would be good to know if ppl are ok with the proposed way of implementing this functionality on Linux, and then we can bring the Windows implementation up to date as well (and add some more tests to this PR).

jiangliu · 2019-06-11T09:15:55Z

src/mmap.rs

+    pub fn new(mapping: MmapRegion, guest_base: GuestAddress) -> result::Result<Self, Error> {
+        if guest_base.0.checked_add(mapping.len() as u64).is_none() {
+            return Err(Error::InvalidGuestRegion);
+        }


Should we also check that the offset and len are page aligned? Or just let the mmap syscall to check them?

I guess it's better to check it ourselves since we cannot assume that both Linux and Windows mmap syscall will check for that, right?

I don't think we should do the checks at this point; this method expects to be passed a valid MmapRegion. For Linux mmap already checks that offset is aligned (like Sebastien pointed out a while ago), and the length of the mapping doesn't need to be page aligned. I don't know what Windows does, but if the equivalent of mmap doesn't check alignment when necessary, our code should do it when creating the Windows MmapRegion.

jiangliu · 2019-06-11T09:16:27Z

src/guest_memory.rs

+        let file = tempfile::tempfile().unwrap();
+        let start = 1234;
+        let file_offset = FileOffset::new(file, start);
+        assert_eq!(file_offset.start(), start);


How about adding a test case for non-page-aligned offset?

I wanted to keep FileOffset as lightweight as possible. If a FileOffset object is invalid for some reason, this will be caught when actually trying to map the memory. I'll add an offset alignment test for MmapRegion if there isn't one already.

rbradford · 2019-06-13T09:40:07Z

@sboeuf What's your thoughts on this?

sboeuf · 2019-06-13T14:50:52Z

@rbradford

@sboeuf What's your thoughts on this?

I'm planning to review this today :)

src/mmap.rs

sboeuf

@alexandruag From a design perspective, the PR looks perfect to me! You can definitely move forward and complete it, unless @jiangliu or @bonzini have some objections.
Thanks!

src/mmap.rs

alexandruag

Hi everyone,

Just finished answering comments and rebasing the PR with the following changes:

Added the arc(&self) method to FileOffset so now file(&self) returns an actual &File.
Renamed fd_overlaps to fds_overlap.
Added tests for mmap::build and mmap::fds_overlap.
Added the test changes from Sebastien's original to the last commit.

There's still the question of what to do on the Windows side. Unfortunately, I didn't have the time to pursue that as well. Will try to reach out to @tkreuzer and see how we can proceed.

alexandruag · 2019-06-17T12:46:10Z

src/guest_memory.rs

+        let file = tempfile::tempfile().unwrap();
+        let start = 1234;
+        let file_offset = FileOffset::new(file, start);
+        assert_eq!(file_offset.start(), start);


I wanted to keep FileOffset as lightweight as possible. If a FileOffset object is invalid for some reason, this will be caught when actually trying to map the memory. I'll add an offset alignment test for MmapRegion if there isn't one already.

alexandruag · 2019-06-17T19:32:51Z

src/mmap.rs

+    pub fn new(mapping: MmapRegion, guest_base: GuestAddress) -> result::Result<Self, Error> {
+        if guest_base.0.checked_add(mapping.len() as u64).is_none() {
+            return Err(Error::InvalidGuestRegion);
+        }


I don't think we should do the checks at this point; this method expects to be passed a valid MmapRegion. For Linux mmap already checks that offset is aligned (like Sebastien pointed out a while ago), and the length of the mapping doesn't need to be page aligned. I don't know what Windows does, but if the equivalent of mmap doesn't check alignment when necessary, our code should do it when creating the Windows MmapRegion.

src/mmap.rs

sboeuf

Looks good, the only thing missing is to adapt the Windows code too. It looks like it should be pretty easy since it's mostly about renaming functions, or am I missing something?

sboeuf · 2019-06-17T21:18:35Z

src/mmap_unix.rs

+    }
+
+    /// Creates a new mapping based on the provided arguments.
+    pub fn build(


I think that half of this function should be generic to unix and windows, therefore should be defined in mmap.rs. The file offset processing is the part that should be common.

Make sense, I refactored this by adding the check_file_offset function to mmap.rs in the same commit.

sboeuf · 2019-06-17T21:19:56Z

src/mmap_unix.rs

-                fd.as_raw_fd(),
-                offset,
-            )
+    pub fn from_file(file_offset: FileOffset, size: usize) -> Result<Self> {


We need to modify the same way the function from_fd in mmap_windows.rs. This should be pretty straightforward.

I pulled in the commit where you did this, thanks!

Added a struct which keeps track of the file and offset associated with a guest memory region backed by a memory mapped file. Signed-off-by: Alexandru Agache <aagch@amazon.com>

alexandruag

Did a bit of refactoring (as mentioned in one of the responses), and cherry-picked a commit from Sebastien which makes the Windows tests succeed. We still need more Windows testing, but the CI should be happy at least.

alexandruag · 2019-06-24T20:06:54Z

src/mmap_unix.rs

-                fd.as_raw_fd(),
-                offset,
-            )
+    pub fn from_file(file_offset: FileOffset, size: usize) -> Result<Self> {


I pulled in the commit where you did this, thanks!

alexandruag · 2019-06-24T21:06:36Z

src/mmap_unix.rs

+    }
+
+    /// Creates a new mapping based on the provided arguments.
+    pub fn build(


Make sense, I refactored this by adding the check_file_offset function to mmap.rs in the same commit.

Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com> Signed-off-by: Alexandru Agache <aagch@amazon.com>

Also, refactored some names and the way we resolve paths to use different `Error` and `Result` definitions. Signed-off-by: Alexandru Agache <aagch@amazon.com>

The GuestRegionMmap constructor now checks that adding the guest base address to the length of the mapped region does not overflow. Signed-off-by: Alexandru Agache <aagch@amazon.com>

The new method simplifies the creation of a GuestMemoryMmap object that contains file backed regions (potentially alongside anonymous memory regions). Also, updated the GuestMemoryRegion trait with the `file_offset` method, and implemented it for GuestRegionMmap. Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com> Signed-off-by: Alexandru Agache <aagch@amazon.com>

Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>

alexandruag · 2019-06-24T21:31:14Z

@bonzini @jiangliu can you please take a look?

jiangliu · 2019-06-25T14:27:31Z

src/mmap.rs

+            if metadata.len() < end {
+                return Err(MmapRegionError::MappingPastEof);
+            }
+        }


Do we need to handle the case when file.metadata() fails?

We could create a special Error case for it.
Once @alexandruag updates it, we need to re-approve the PR.

We could merge the big PR first and submit another bugfix PR for this issue.
Review a small bugfix PR is much easier:)

sboeuf · 2019-06-25T21:36:02Z

The green button is very tempting :)

jiangliu reviewed Jun 11, 2019

View reviewed changes

alexandruag requested a review from bonzini as a code owner June 11, 2019 09:51

sboeuf reviewed Jun 13, 2019

View reviewed changes

src/mmap.rs Show resolved Hide resolved

sboeuf previously approved these changes Jun 13, 2019

View reviewed changes

sboeuf reviewed Jun 13, 2019

View reviewed changes

src/mmap.rs Show resolved Hide resolved

This was referenced Jun 14, 2019

Adopt the vm-memory crate from rust-vmm firecracker-microvm/firecracker#1137

Closed

Publish on crates.io #31

Closed

sboeuf mentioned this pull request Jun 14, 2019

Add virtio-fs support cloud-hypervisor/cloud-hypervisor#30

Merged

alexandruag commented Jun 17, 2019

View reviewed changes

alexandruag dismissed sboeuf’s stale review via de979be June 17, 2019 20:51

alexandruag force-pushed the file_backed_regions branch from 19657f7 to de979be Compare June 17, 2019 20:51

sboeuf previously approved these changes Jun 17, 2019

View reviewed changes

This was referenced Jun 20, 2019

Create types of memory regions #34

Closed

[DNM] Fix windows for PR #29 #35

Closed

guest_memory: added FileOffset struct

dc8da86

Added a struct which keeps track of the file and offset associated with a guest memory region backed by a memory mapped file. Signed-off-by: Alexandru Agache <aagch@amazon.com>

alexandruag commented Jun 24, 2019

View reviewed changes

alexandruag dismissed sboeuf’s stale review via c5e6d2a June 24, 2019 21:08

alexandruag force-pushed the file_backed_regions branch from de979be to c5e6d2a Compare June 24, 2019 21:08

alexandruag and others added 5 commits June 25, 2019 00:12

mmap_unix: added new functionality

b81e3be

Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com> Signed-off-by: Alexandru Agache <aagch@amazon.com>

mmap: get rid of "use guest_memory::*"

a684b17

Also, refactored some names and the way we resolve paths to use different `Error` and `Result` definitions. Signed-off-by: Alexandru Agache <aagch@amazon.com>

GuestRegionMmap: added error check to constructor

c11e453

The GuestRegionMmap constructor now checks that adding the guest base address to the length of the mapped region does not overflow. Signed-off-by: Alexandru Agache <aagch@amazon.com>

mmap: Fix windows implementation and tests

e6e235e

Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>

alexandruag force-pushed the file_backed_regions branch from c5e6d2a to e6e235e Compare June 24, 2019 21:12

sboeuf approved these changes Jun 24, 2019

View reviewed changes

jiangliu approved these changes Jun 25, 2019

View reviewed changes

jiangliu merged commit 4c329f4 into rust-vmm:master Jun 26, 2019

sboeuf mentioned this pull request Jun 26, 2019

Facilitate the creation of file backed guest memory regions #20

Closed

Another iteration of supporting file backed memory regions #29

Another iteration of supporting file backed memory regions #29

Uh oh!

Conversation

alexandruag commented Jun 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbradford commented Jun 13, 2019

Uh oh!

sboeuf commented Jun 13, 2019

Uh oh!

Uh oh!

sboeuf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexandruag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sboeuf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexandruag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexandruag commented Jun 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sboeuf commented Jun 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants