Facilitate the creation of file backed guest memory regions #20

sboeuf · 2019-05-22T19:30:42Z

In order to support use cases where the memory needs to be backed by
actual files in order to be shared across processes (vhost-user case),
this patch introduces a new function creating the guest memory with
each region backed by a file descriptor provided by the caller.

It also introduces two helpers to retrieve file information related
to a region. Each file is owned by the memory region so that the
caller does not have to deal with ownership. This way, the file gets
to live as long as the memory region is active.

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

sboeuf · 2019-05-22T19:33:37Z

@jiangliu @bonzini @sameo PTAL

This is an attempt to initialize the guest memory the same way it can be done now, but adding the facility to provide a file and an offset for each region.

rbradford

Minor feedback.

src/mmap.rs

rbradford · 2019-05-23T15:04:01Z

src/mmap.rs

    mapping: MmapRegion,
    guest_base: GuestAddress,
+    file: Option<File>,
+    offset: Option<usize>,


I think this should be MemoryRegionAddress

@jiangliu @bonzini do you have any preference here?

I think usize is fine for offset here because it's a characteristic of the mmaped file on the host (its original type being off_t, which fits nicely within an usize).

Maybe a (file, off_t) pair is more staightforward:)

alexandruag

Did not have a very good look at the tests, will do another pass later. Just wanted to post some preliminary comments.

alexandruag · 2019-05-26T13:47:52Z

src/guest_memory.rs


+    /// Return the file descriptor pointing to the memory region. Return None
+    /// if no file descriptor is associated with the region.
+    fn fd(&self) -> Option<RawFd>;


Since fd and fd_offset are either both Some(...) or both None, I think it would be nicer to have a single method that returns an Option<(RawFd, usize)>.

Also, just wondering, what's the use case for having them as part of the GuestMemoryRegion trait? If none of the vm-memory consumers actually needs something like this, I would remove them for now.

Btw, I think RawFd doesn't work for Windows; we'll either have to abstract this (like provide a reference to the actual File or smt), or conditionally compile different versions of the method for different platforms.

Also, just wondering, what's the use case for having them as part of the GuestMemoryRegion trait? If none of the vm-memory consumers actually needs something like this, I would remove them for now.

Needed for vhost.

Since fd and fd_offset are either both Some(...) or both None, I think it would be nicer to have a single method that returns an Option<(RawFd, usize)>.

Yes I agree, I'll change that.

Also, just wondering, what's the use case for having them as part of the GuestMemoryRegion trait? If none of the vm-memory consumers actually needs something like this, I would remove them for now.

Here is a consumer cloud-hypervisor/cloud-hypervisor#30 ;)

Btw, I think RawFd doesn't work for Windows; we'll either have to abstract this (like provide a reference to the actual File or smt), or conditionally compile different versions of the method for different platforms.

Ah yes, what's the preferred way to do this? @sameo @rbradford @jiangliu

Well, one way of not being biased towards unix stuff in this particular case is to return an Option<&File> instead of Option<RawFd> (assuming you have a File object somewhere, which seems to be the case).

Both Raw and File should be OK, but not AsRawFd because it's not Send ready. We could easily construct a File object from RawFd and File is Send/Sync.

Well we would not have to construct the File if we were actually keeping the File. WDYT?

Yeah, I mean File is as convenient as RawFd, and it's more friend to Windows:)

Hi, I've created a PR here with some changes on top of Sebastien's branch that embody the proposals I've made. I don't know where those conflicts came from :( Still, it would be great if you ppl can have a look.

File is better not really because of Windows, since this is meant for SCM_RIGHTS support, but rather because it allows tracking the lifetime correctly. What about a separate struct FileRange:

struct FileRange { file: Arc<File>, offset: u64, size: usize };

and having a single method that returns Option<FileRange> here?

alexandruag · 2019-05-26T13:53:10Z

src/mmap.rs

 pub struct GuestRegionMmap {
    mapping: MmapRegion,
    guest_base: GuestAddress,
+    file: Option<File>,


I think it's a bit more natural to have file and offset as members of MmapRegion, since they are basically parameters of the mmaped host memory region, and don't have anything to do with the guest. Also, it might be nicer to have a single Option<(File, usize)> instead of two separate Options.

@sboeuf Can you investigate MmapRegion instead? It sounds logical. I think if you move it it would also be possible to switch to the @alexandruag's proposed single Option<File, usize>

I think this is feasible, but do you expect me to implement this both for mmap_unix.rs and mmap_windows.rs?

I don't really like to merge the file and the file offset into a tuple, because it's less readable from a code perspective IMO.

Just for mmap_unix.rs, since in its current state this source file is Unix-specific anyway :P We can reach out to the Windows folks regarding also updating the mmap_windows.rs file.

Btw, if we get vote on readability I like the tuple more, otherwise u end up with gratuitous unwraps just to get a value which you know is present :D

Ah yes you got a point with the unwrap here!

I remember one of the reason I didn't create File as part of the MmapRegion. I wanted the region to own the File. And to do so, I had to invoke try_clone(), which should not be done at the same level the mmap() is done IMO.
Both unix and windows should share the same ownership behavior regarding the File I think.

@jiangliu what do you think about this?

It's feasible to move (file, offset) pair into MmapRegion. I'm also thinking of adding an method to the GuestMemoryRegion trait to expose the (file, offset) info. Then clients like vhost may depend on GuestMemoryRegion instead of GuestRegionMmap.

alexandruag · 2019-05-26T13:56:26Z

src/mmap.rs

    mapping: MmapRegion,
    guest_base: GuestAddress,
+    file: Option<File>,
+    offset: Option<usize>,


I think usize is fine for offset here because it's a characteristic of the mmaped file on the host (its original type being off_t, which fits nicely within an usize).

alexandruag · 2019-05-26T14:04:06Z

src/mmap.rs

+    /// Creates a container and allocates shared memory backed by a file for
+    /// guest memory regions. Valid memory regions are specified as a Vec of
+    /// (Address, Size, File, Offset) tuples sorted by Address.
+    pub fn new_backed_by_file(


I guess this is mainly a forward looking comment, in the sense it doesn't necessarily fit in the scope of this PR, but it feels a bit redundant to have three constructors for GuestMemoryMmap, which also share quite a bit of logic (like the overlapping regions check). I think it would be neat to only keep from_regions, and then implement some helper logic in mmap_unix.rs (and the Windows one I guess) to simplify creating Vecs of regions (file backed or not), which can then be passed to from_regions. Just throwing this out there, what do you ppl think?

Yes I agree with this one, but let's do it in 2 steps if you don't mind. Let's first introduce the new possibility to create memory regions backed by a file, and then we can try to provide all features through a factorized function.

Let's add the FileRange struct, and make this take a &[(GuestAddress, FileRange)].

@alexandruag will include it in its PR.

In order to support use cases where the memory needs to be backed by actual files in order to be shared across processes (vhost-user case), this patch introduces a new function creating the guest memory with each region backed by a file descriptor provided by the caller. It also introduces two helpers to retrieve file information related to a region. Each file is owned by the memory region so that the caller does not have to deal with ownership. This way, the file gets to live as long as the memory region is active. Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>

bonzini · 2019-06-04T13:22:49Z

src/mmap.rs

+    /// Creates a container and allocates shared memory backed by a file for
+    /// guest memory regions. Valid memory regions are specified as a Vec of
+    /// (Address, Size, File, Offset) tuples sorted by Address.
+    pub fn new_backed_by_file(


Let's add the FileRange struct, and make this take a &[(GuestAddress, FileRange)].

sboeuf · 2019-06-26T23:01:31Z

Closing as superseded by #29 which got merged!

sboeuf requested review from bonzini and jiangliu May 22, 2019 19:30

sboeuf force-pushed the add_fd_support branch 2 times, most recently from 19560fb to 19733a4 Compare May 22, 2019 20:31

sboeuf mentioned this pull request May 22, 2019

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

Merged

rbradford previously approved these changes May 23, 2019

View reviewed changes

sboeuf dismissed rbradford’s stale review via 9c88452 May 23, 2019 15:23

sboeuf force-pushed the add_fd_support branch 2 times, most recently from 9c88452 to 1932cb6 Compare May 23, 2019 15:29

sboeuf requested a review from andreeaflorescu May 24, 2019 23:25

alexandruag reviewed May 26, 2019

View reviewed changes

sboeuf force-pushed the add_fd_support branch from c66c678 to f256c57 Compare June 3, 2019 22:00

bonzini requested changes Jun 4, 2019

View reviewed changes

alexandruag mentioned this pull request Jun 11, 2019

Another iteration of supporting file backed memory regions #29

Merged

sboeuf closed this Jun 26, 2019

Facilitate the creation of file backed guest memory regions #20

Facilitate the creation of file backed guest memory regions #20

Uh oh!

Conversation

sboeuf commented May 22, 2019

Uh oh!

sboeuf commented May 22, 2019

Uh oh!

rbradford left a comment

Choose a reason for hiding this comment

Uh oh!

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!

jiangliu May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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 May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sboeuf commented Jun 26, 2019

Uh oh!

Reviewers

Assignees

Labels

jiangliu May 31, 2019 •

edited

Loading

alexandruag May 28, 2019 •

edited

Loading