Refactoring around file base mappings #2
Conversation
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>
Signed-off-by: Alexandru Agache <aagch@amazon.com>
| #[derive(Debug)] | ||
| pub struct FileMappingConfig { | ||
| file: File, | ||
| offset: usize, |
There was a problem hiding this comment.
How about using libc::off_t instead of usize here? Offset into file object may be differrent from usize.
There was a problem hiding this comment.
I would keep usize at the guest_memory level, in order to keep the cast into libc::off_t internal to the crate.
There was a problem hiding this comment.
Ultimately, like Paolo mentioned on the discussions around the original PR, usize turns into a bad idea here because of files larger than 4G on 32 bit systems. I'll prob end up using a type alias for offset which will be equivalent to off_t on Linux, and something else on Windows, just to have a clean interface.
| // even if we do verify, someone might truncate the file later. I guess it might still be a | ||
| // valuable sanity check. | ||
| // TODO: should we explicitly check that the offset is a multiple of PAGE_SIZE, or will mmap return | ||
| // an error anyway in this case? |
There was a problem hiding this comment.
mmap will throw you an error, no need to check for this.
There was a problem hiding this comment.
Ah right, didn't remember the documentation actually mentions this.
sboeuf
left a comment
There was a problem hiding this comment.
@alexandruag this looks pretty good to me. Unless @jiangliu has some objections, I would say go ahead and finish the implementation that you started here. You can squash with my commit if you want I don't mind.
One comment though, you could have gone one step further by having a single function new() that would always take a tuple (GuestAddress, length, Option<file, offset>). This way, we would give the consumer the way to initialize non-homogeneous GuestMemoryMmap. From a single function we could have a GuestMemoryMmap describing both some non-backed and backed memory ranges. WDYT?
I like these changes:) |
|
@sboeuf You're right that sounds good, this was just like a minimal proof of concept implementation. Btw, when squashing commits how can I ensure all the authors show up in the history? |
Add |
The changes I propose are all in the "refactoring around file-backed mappings" commit. Here's the gist of things:
FileMappingConfigstruct toguest_memory.rs. This structure holds the configuration of a file backed memory region. It currently only consists of the file and offset, but later it might also record things like flags, and more complex mapping configurations if we ever decide we'd like to have one region backed by multiple mappings.TODOs.If you'd like to keep these changes, I can add everything that's missing and we can merge them.