Hi,
patch subject should start with "softmmu/physmem: Open ..."
On 25.07.23 12:52, Thiner Logoer wrote:
An read only file can be mapped with read write as long as the
mapping is private, which is very common case. Make
At least in the environments I know, using private file mappings is a corner
case ;)
What is you use case? VM templating?
qemu_ram_alloc_from_file open file as read only when the
mapping is private, otherwise open will fail when file
does not allow write.
If this file does not exist or is a directory, the flag is not used,
so it should be OK.
from https://gitlab.com/qemu-project/qemu/-/issues/1689
Signed-off-by: Thiner Logoer <logoerthin...@163.com>
---
softmmu/physmem.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 3df73542e1..e8036ee335 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -1945,8 +1945,15 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size,
MemoryRegion *mr,
int fd;
bool created;
RAMBlock *block;
+
^
.git/rebase-apply/patch:13: trailing whitespace.
+ /*
+ * If map is private, the fd does not need to be writable.
+ * This only get effective when the file is existent.
"This will get ignored if the file does not yet exist."
+ */
+ bool open_as_readonly = readonly || !(ram_flags & RAM_SHARED);
- fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created,
+ fd = file_ram_open(mem_path, memory_region_name(mr),
+ open_as_readonly, &created,
errp);
if (fd < 0) {
return NULL;
Opening a file R/O will also make operations like fallocate/ftruncate/ ... fail.
For example, this will make fallocate(FALLOC_FL_PUNCH_HOLE) stop working and in
turn make ram_block_discard_range() bail out.
There was a recent discussion/patch on that:
commit 1d44ff586f8a8e113379430750b5a0a2a3f64cf9
Author: David Hildenbrand <da...@redhat.com>
Date: Thu Jul 6 09:56:06 2023 +0200
softmmu/physmem: Warn with ram_block_discard_range() on MAP_PRIVATE file
mapping
ram_block_discard_range() cannot possibly do the right thing in
MAP_PRIVATE file mappings in the general case.
To achieve the documented semantics, we also have to punch a hole into
the file, possibly messing with other MAP_PRIVATE/MAP_SHARED mappings
of such a file.
For example, using VM templating -- see commit b17fbbe55cba ("migration:
allow private destination ram with x-ignore-shared") -- in combination with
any mechanism that relies on discarding of RAM is problematic. This
includes:
* Postcopy live migration
* virtio-balloon inflation/deflation or free-page-reporting
* virtio-mem
So at least warn that there is something possibly dangerous is going on
when using ram_block_discard_range() in these cases.
While it doesn't work "in the general case", it works in the "single file
owner" case
where someone simply forgot to specify "share=on" -- "share=off" is the default
for
memory-backend-file :( .
For example, with hugetlb+virtio-mem the following works if the file does not
exists:
(note that virtio-mem will fallocate(FALLOC_FL_PUNCH_HOLE) the whole file
upfront)
...
-object
memory-backend-file,share=off,mem-path=/dev/hugepages/vmem0,id=mem2,size=2G \
-device virtio-mem-pci,id=vmem0,memdev=mem2,requested-size=1g,bus=root
With you patch, once the file already exists, we would now get
qemu-system-x86_64: -device
virtio-mem-pci,id=vmem0,memdev=mem2,requested-size=1g,bus=root:
ram_block_discard_range: Failed to fallocate :0 +80000000 (-9)
qemu-system-x86_64: -device
virtio-mem-pci,id=vmem0,memdev=mem2,requested-size=1g,bus=root: Unexpected
error discarding RAM: Bad file descriptor
So this has the potential to break existing setups.
The easy fix for these would be to configure "share=on" in these now-failing
setups. Hmmmmm ....
--
Cheers,
David / dhildenb
