On Mon, Aug 07, 2023 at 09:07:32PM +0200, David Hildenbrand wrote:
> From: Thiner Logoer <logoerthin...@163.com>
>
> Users may specify
> * "-mem-path" or
> * "-object memory-backend-file,share=off,readonly=off"
> and expect such COW (MAP_PRIVATE) mappings to work, even if the user
> does not have write permissions to open the file.
>
> For now, we would always fail in that case, always requiring file write
> permissions. Let's detect when that failure happens and fallback to opening
> the file readonly.
>
> Warn the user, since there are other use cases where we want the file to
> be mapped writable: ftruncate() and fallocate() will fail if the file
> was not opened with write permissions.
>
> Signed-off-by: Thiner Logoer <logoerthin...@163.com>
> Co-developed-by: David Hildenbrand <da...@redhat.com>
> Signed-off-by: David Hildenbrand <da...@redhat.com>
> ---
> softmmu/physmem.c | 26 ++++++++++++++++++--------
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 3df73542e1..d1ae694b20 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -1289,8 +1289,7 @@ static int64_t get_file_align(int fd)
> static int file_ram_open(const char *path,
> const char *region_name,
> bool readonly,
> - bool *created,
> - Error **errp)
> + bool *created)
> {
> char *filename;
> char *sanitized_name;
> @@ -1334,10 +1333,7 @@ static int file_ram_open(const char *path,
> g_free(filename);
> }
> if (errno != EEXIST && errno != EINTR) {
> - error_setg_errno(errp, errno,
> - "can't open backing store %s for guest RAM",
> - path);
> - return -1;
> + return -errno;
> }
> /*
> * Try again on EINTR and EEXIST. The latter happens when
> @@ -1946,9 +1942,23 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size,
> MemoryRegion *mr,
> bool created;
> RAMBlock *block;
>
> - fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created,
> - errp);
> + fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created);
> + if (fd == -EACCES && !(ram_flags & RAM_SHARED) && !readonly) {
> + /*
> + * We can have a writable MAP_PRIVATE mapping of a readonly file.
> + * However, some operations like ftruncate() or fallocate() might
> fail
> + * later, let's warn the user.
> + */
> + fd = file_ram_open(mem_path, memory_region_name(mr), true, &created);
> + if (fd >= 0) {
> + warn_report("backing store %s for guest RAM (MAP_PRIVATE) opened"
> + " readonly because the file is not writable",
> mem_path);
I can understand the use case, but this will be slightly unwanted,
especially the user doesn't yet have a way to predict when will it happen.
Meanwhile this changes the behavior, is it a concern that someone may want
to rely on current behavior of failing?
To think from a higher level of current use case, the ideal solution seems
to me that if the ram file can be put on a file system that supports CoW
itself (like btrfs), we can snapshot that ram file and make it RW for the
qemu instance. Then here it'll be able to open the file. We'll be able to
keep the interface working as before, meanwhile it'll work with fallocate
or truncations too I assume.
Would that be better instead of changing QEMU?
Thanks,
> + }
> + }
> if (fd < 0) {
> + error_setg_errno(errp, -fd,
> + "can't open backing store %s for guest RAM",
> + mem_path);
> return NULL;
> }
>
> --
> 2.41.0
>
--
Peter Xu
