[Qemu-devel] [PATCH 0/3] hostmem-file: make option 'size' optional

[Qemu-devel] [PATCH 0/3] hostmem-file: make option 'size' optional

[Haozhong Zhang]

This patch series is mostly the followup of patch 3 in
"[PATCH v2 0/3] Improve truncation behavior of memory-backend-file" [1][2].

For certain usages of memory-backend-file, users simply want to use
the entire backend file (specified by the "mem-path" option). Therefore,
it's not necessary in such cases to specify the file size via the
"size" option. This patch series makes the "size" option of
memory-backend-file optional. In detail,

1. if the size of the backend file is not zero and the "size" option
   is not present, QEMU will use the file size as the "size" option;

2. if the backend file is a non-existing file, an empty file, or a
   directory, the "size" option is still needed;

3. the "size" option is also needed in cases that QEMU fails to the
   backend file size.

Patch 1 is not quite relevant. It adds missing comments on errors
of get_file_size() per Eduardo's suggestion.

Patch 2 moves the file operations from file_ram_alloc() to
qemu_ram_alloc_from_file(), so that all checks on the file size and
the "size" option, and the related operations on ram block length can
be put in qemu_ram_alloc_from_file() and no duplication is needed.

Patch 3 actually implements the logic to make the option "size" optional.

[1] https://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg06823.html
[2] https://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg00251.html

Haozhong Zhang (3):
  exec.c: add comment for errors of get_file_size()
  exec.c: move file operations to qemu_ram_alloc_from_file()
  hostmem-file: make option 'size' optional

 backends/hostmem-file.c |  28 ++++--
 exec.c                  | 242 ++++++++++++++++++++++++++++++------------------
 2 files changed, 170 insertions(+), 100 deletions(-)

-- 
2.10.1

[Qemu-devel] [PATCH 1/3] exec.c: add comment for errors of get_file_size()

[Haozhong Zhang]

Indicate that not stopping on get_file_size() errors in file_ram_alloc()
is on purpose and not a mistake.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 exec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/exec.c b/exec.c
index 3d867f1..68b0c92 100644
--- a/exec.c
+++ b/exec.c
@@ -1314,6 +1314,9 @@ static void *file_ram_alloc(RAMBlock *block,
     }
 #endif
 
+    /* If QEMU fails to get the backend file size, i.e. file_size < 0,
+     * it will treat the file as non-empty and not truncate it.
+     */
     file_size = get_file_size(fd);
 
     if (memory < block->page_size) {
-- 
2.10.1

[Qemu-devel] [PATCH 2/3] exec.c: move file operations to qemu_ram_alloc_from_file()

[Haozhong Zhang]

Move operations that open/close/get size of the backend file from
file_ram_alloc() to its caller qemu_ram_alloc_from_file(). After this
change, check of backend file size and memory size can be put in
qemu_ram_alloc_from_file().

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 exec.c | 218 +++++++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 123 insertions(+), 95 deletions(-)

diff --git a/exec.c b/exec.c
index 68b0c92..42045aa 100644
--- a/exec.c
+++ b/exec.c
@@ -1231,27 +1231,13 @@ void qemu_mutex_unlock_ramlist(void)
 }
 
 #ifdef __linux__
-static int64_t get_file_size(int fd)
-{
-    int64_t size = lseek(fd, 0, SEEK_END);
-    if (size < 0) {
-        return -errno;
-    }
-    return size;
-}
-
 static void *file_ram_alloc(RAMBlock *block,
                             ram_addr_t memory,
-                            const char *path,
+                            int fd,
+                            bool truncate,
                             Error **errp)
 {
-    bool unlink_on_error = false;
-    char *filename;
-    char *sanitized_name;
-    char *c;
     void *area = MAP_FAILED;
-    int fd = -1;
-    int64_t file_size;
 
     if (kvm_enabled() && !kvm_has_sync_mmu()) {
         error_setg(errp,
@@ -1259,53 +1245,6 @@ static void *file_ram_alloc(RAMBlock *block,
         return NULL;
     }
 
-    for (;;) {
-        fd = open(path, O_RDWR);
-        if (fd >= 0) {
-            /* @path names an existing file, use it */
-            break;
-        }
-        if (errno == ENOENT) {
-            /* @path names a file that doesn't exist, create it */
-            fd = open(path, O_RDWR | O_CREAT | O_EXCL, 0644);
-            if (fd >= 0) {
-                unlink_on_error = true;
-                break;
-            }
-        } else if (errno == EISDIR) {
-            /* @path names a directory, create a file there */
-            /* Make name safe to use with mkstemp by replacing '/' with '_'. */
-            sanitized_name = g_strdup(memory_region_name(block->mr));
-            for (c = sanitized_name; *c != '\0'; c++) {
-                if (*c == '/') {
-                    *c = '_';
-                }
-            }
-
-            filename = g_strdup_printf("%s/qemu_back_mem.%s.XXXXXX", path,
-                                       sanitized_name);
-            g_free(sanitized_name);
-
-            fd = mkstemp(filename);
-            if (fd >= 0) {
-                unlink(filename);
-                g_free(filename);
-                break;
-            }
-            g_free(filename);
-        }
-        if (errno != EEXIST && errno != EINTR) {
-            error_setg_errno(errp, errno,
-                             "can't open backing store %s for guest RAM",
-                             path);
-            goto error;
-        }
-        /*
-         * Try again on EINTR and EEXIST.  The latter happens when
-         * something else creates the file between our two open().
-         */
-    }
-
     block->page_size = qemu_fd_getpagesize(fd);
     block->mr->align = block->page_size;
 #if defined(__s390x__)
@@ -1314,11 +1253,6 @@ static void *file_ram_alloc(RAMBlock *block,
     }
 #endif
 
-    /* If QEMU fails to get the backend file size, i.e. file_size < 0,
-     * it will treat the file as non-empty and not truncate it.
-     */
-    file_size = get_file_size(fd);
-
     if (memory < block->page_size) {
         error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
                    "or larger than page size 0x%zx",
@@ -1326,13 +1260,6 @@ static void *file_ram_alloc(RAMBlock *block,
         goto error;
     }
 
-    if (file_size > 0 && file_size < memory) {
-        error_setg(errp, "backing store %s size 0x%" PRIx64
-                   " does not match 'size' option 0x" RAM_ADDR_FMT,
-                   path, file_size, memory);
-        goto error;
-    }
-
     memory = ROUND_UP(memory, block->page_size);
 
     /*
@@ -1340,16 +1267,8 @@ static void *file_ram_alloc(RAMBlock *block,
      * hosts, so don't bother bailing out on errors.
      * If anything goes wrong with it under other filesystems,
      * mmap will fail.
-     *
-     * Do not truncate the non-empty backend file to avoid corrupting
-     * the existing data in the file. Disabling shrinking is not
-     * enough. For example, the current vNVDIMM implementation stores
-     * the guest NVDIMM labels at the end of the backend file. If the
-     * backend file is later extended, QEMU will not be able to find
-     * those labels. Therefore, extending the non-empty backend file
-     * is disabled as well.
      */
-    if (!file_size && ftruncate(fd, memory)) {
+    if (truncate && ftruncate(fd, memory)) {
         perror("ftruncate");
     }
 
@@ -1375,12 +1294,6 @@ error:
     if (area != MAP_FAILED) {
         qemu_ram_munmap(area, memory);
     }
-    if (unlink_on_error) {
-        unlink(path);
-    }
-    if (fd != -1) {
-        close(fd);
-    }
     return NULL;
 }
 #endif
@@ -1674,11 +1587,95 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
 }
 
 #ifdef __linux__
+/*
+ * Open the backend file @path and return the file descriptor on
+ * success.
+ *
+ * If @path does not exist, it will be created and true will be
+ * returned via @unlink_on_error.
+ *
+ * If @path is a directory, a temporal file will be created under
+ * @path and its file descriptor will be returned.
+ *
+ * @errp will be set on any errors.
+ */
+static int backend_file_open(const char *path, const char *name,
+                             bool *unlink_on_error, Error **errp)
+{
+    char *filename;
+    char *sanitized_name;
+    char *c;
+    int fd = -1;
+    Error *local_err = NULL;
+
+    for (;;) {
+        fd = open(path, O_RDWR);
+        if (fd >= 0) {
+            /* @path names an existing file, use it */
+            break;
+        }
+        if (errno == ENOENT) {
+            /* @path names a file that doesn't exist, create it */
+            fd = open(path, O_RDWR | O_CREAT | O_EXCL, 0644);
+            if (fd >= 0) {
+                *unlink_on_error = true;
+                break;
+            }
+        } else if (errno == EISDIR) {
+            /* @path names a directory, create a file there */
+            /* Make name safe to use with mkstemp by replacing '/' with '_'. */
+            sanitized_name = g_strdup(name);
+            for (c = sanitized_name; *c != '\0'; c++) {
+                if (*c == '/') {
+                    *c = '_';
+                }
+            }
+
+            filename = g_strdup_printf("%s/qemu_back_mem.%s.XXXXXX", path,
+                                       sanitized_name);
+            g_free(sanitized_name);
+
+            fd = mkstemp(filename);
+            if (fd >= 0) {
+                unlink(filename);
+                g_free(filename);
+                break;
+            }
+            g_free(filename);
+        }
+        if (errno != EEXIST && errno != EINTR) {
+            error_setg_errno(&local_err, errno,
+                             "can't open backing store %s for guest RAM",
+                             path);
+            break;
+        }
+        /*
+         * Try again on EINTR and EEXIST.  The latter happens when
+         * something else creates the file between our two open().
+         */
+    }
+
+    error_propagate(errp, local_err);
+    return fd;
+}
+
+static int64_t get_file_size(int fd)
+{
+    int64_t size = lseek(fd, 0, SEEK_END);
+    if (size < 0) {
+        return -errno;
+    }
+    return size;
+}
+
 RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
                                    bool share, const char *mem_path,
                                    Error **errp)
 {
-    RAMBlock *new_block;
+    RAMBlock *new_block = NULL;
+    int fd = -1;
+    bool unlink_on_error = false;
+    int64_t file_size;
     Error *local_err = NULL;
 
     if (xen_enabled()) {
@@ -1697,22 +1694,53 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
         return NULL;
     }
 
+    fd = backend_file_open(mem_path, memory_region_name(mr), &unlink_on_error,
+                           &local_err);
+    if (fd < 0) {
+        goto out;
+    }
+    /* If QEMU fails to get the backend file size, i.e. file_size < 0,
+     * it will treat the file as non-empty and not truncate it.
+     */
+    file_size = get_file_size(fd);
+
     size = HOST_PAGE_ALIGN(size);
+    if (file_size > 0 && file_size < size) {
+        error_setg(&local_err, "backing store %s size 0x%"PRIx64
+                   " does not match 'size' option 0x"RAM_ADDR_FMT,
+                   mem_path, file_size, size);
+        goto out;
+    }
+
     new_block = g_malloc0(sizeof(*new_block));
     new_block->mr = mr;
     new_block->used_length = size;
     new_block->max_length = size;
     new_block->flags = share ? RAM_SHARED : 0;
-    new_block->host = file_ram_alloc(new_block, size,
-                                     mem_path, errp);
+    /* Do not truncate the non-empty backend file (i.e. file_size != 0)
+     * to avoid corrupting the existing data in the file. Disabling
+     * shrinking is not enough. For example, the current vNVDIMM
+     * implementation stores the guest NVDIMM labels at the end of the
+     * backend file. If the backend file is later extended, QEMU will
+     * not be able to find those labels. Therefore, extending the
+     * non-empty backend file is disabled as well.
+     */
+    new_block->host = file_ram_alloc(new_block, size, fd, !file_size, errp);
     if (!new_block->host) {
-        g_free(new_block);
-        return NULL;
+        goto out;
     }
 
     ram_block_add(new_block, &local_err);
+
+ out:
     if (local_err) {
         g_free(new_block);
+        if (unlink_on_error) {
+            unlink(mem_path);
+        }
+        if (fd >= 0) {
+            close(fd);
+        }
         error_propagate(errp, local_err);
         return NULL;
     }
-- 
2.10.1

[Qemu-devel] [PATCH 3/3] hostmem-file: make option 'size' optional

[Haozhong Zhang]

If 'size' option is not specified, QEMU will use the file size of
'mem-path' instead. If an empty file, a non-existing file or a directory
is specified by option 'mem-path', a non-zero option 'size' is still
needed.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 backends/hostmem-file.c | 28 ++++++++++++++++++++--------
 exec.c                  | 27 +++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 42efb2f..56cc96b 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -39,17 +39,14 @@ static void
 file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 {
     HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
+    Error *local_err = NULL;
 
-    if (!backend->size) {
-        error_setg(errp, "can't create backend with size 0");
-        return;
-    }
     if (!fb->mem_path) {
-        error_setg(errp, "mem-path property not set");
-        return;
+        error_setg(&local_err, "mem-path property not set");
+        goto out;
     }
 #ifndef CONFIG_LINUX
-    error_setg(errp, "-mem-path not supported on this host");
+    error_setg(&local_err, "-mem-path not supported on this host");
 #else
     if (!memory_region_size(&backend->mr)) {
         gchar *path;
@@ -58,10 +55,25 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
         memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
                                  path,
                                  backend->size, fb->share,
-                                 fb->mem_path, errp);
+                                 fb->mem_path, &local_err);
         g_free(path);
+
+        if (local_err) {
+            goto out;
+        }
+
+        if (!backend->size) {
+            backend->size = memory_region_size(&backend->mr);
+        }
+    }
+
+    if (!backend->size) {
+        error_setg(&local_err, "can't create backend with size 0");
     }
 #endif
+
+ out:
+    error_propagate(errp, local_err);
 }
 
 static char *get_mem_path(Object *o, Error **errp)
diff --git a/exec.c b/exec.c
index 42045aa..b81c9a8 100644
--- a/exec.c
+++ b/exec.c
@@ -1676,6 +1676,7 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
     int fd = -1;
     bool unlink_on_error = false;
     int64_t file_size;
+    uint64_t mr_size;
     Error *local_err = NULL;
 
     if (xen_enabled()) {
@@ -1704,6 +1705,32 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
      */
     file_size = get_file_size(fd);
 
+    if (!size && file_size >= 0) {
+        if (!file_size) {
+            error_setg(&local_err,
+                       "%s is empty or a directory, 'size' option must be specified",
+                       mem_path);
+            goto out;
+        }
+
+        mr_size = memory_region_size(mr);
+        if (mr_size && mr_size != size) {
+            error_setg(&local_err, "cannot resize non-empty memory region");
+            goto out;
+        }
+        if (!mr_size) {
+            memory_region_set_size(mr, file_size);
+        }
+        size = file_size;
+    }
+
+    if (!size) {
+        error_setg(&local_err,
+                   "cannot get size of %s, 'size' option must be specified",
+                   mem_path);
+        goto out;
+    }
+
     size = HOST_PAGE_ALIGN(size);
     if (file_size > 0 && file_size < size) {
         error_setg(&local_err, "backing store %s size 0x%"PRIx64
-- 
2.10.1

Re: [Qemu-devel] [PATCH 0/3] hostmem-file: make option 'size' optional

[Markus Armbruster]

Haozhong Zhang <haozhong.zhang@intel.com> writes:

> This patch series is mostly the followup of patch 3 in
> "[PATCH v2 0/3] Improve truncation behavior of memory-backend-file" [1][2].
>
> For certain usages of memory-backend-file, users simply want to use
> the entire backend file (specified by the "mem-path" option). Therefore,
> it's not necessary in such cases to specify the file size via the
> "size" option. This patch series makes the "size" option of
> memory-backend-file optional. In detail,
>
> 1. if the size of the backend file is not zero and the "size" option
>    is not present, QEMU will use the file size as the "size" option;
>
> 2. if the backend file is a non-existing file, an empty file, or a
>    directory, the "size" option is still needed;
>
> 3. the "size" option is also needed in cases that QEMU fails to the
>    backend file size.

"fails to the backend file size": are you missing a verb?

> Patch 1 is not quite relevant. It adds missing comments on errors
> of get_file_size() per Eduardo's suggestion.
>
> Patch 2 moves the file operations from file_ram_alloc() to
> qemu_ram_alloc_from_file(), so that all checks on the file size and
> the "size" option, and the related operations on ram block length can
> be put in qemu_ram_alloc_from_file() and no duplication is needed.
>
> Patch 3 actually implements the logic to make the option "size" optional.
>
> [1] https://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg06823.html
> [2] https://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg00251.html
>
> Haozhong Zhang (3):
>   exec.c: add comment for errors of get_file_size()
>   exec.c: move file operations to qemu_ram_alloc_from_file()
>   hostmem-file: make option 'size' optional
>
>  backends/hostmem-file.c |  28 ++++--
>  exec.c                  | 242 ++++++++++++++++++++++++++++++------------------
>  2 files changed, 170 insertions(+), 100 deletions(-)

Re: [Qemu-devel] [PATCH 0/3] hostmem-file: make option 'size' optional

[Haozhong Zhang]

On 11/07/16 08:52 +0100, Markus Armbruster wrote:
>Haozhong Zhang <haozhong.zhang@intel.com> writes:
>
>> This patch series is mostly the followup of patch 3 in
>> "[PATCH v2 0/3] Improve truncation behavior of memory-backend-file" [1][2].
>>
>> For certain usages of memory-backend-file, users simply want to use
>> the entire backend file (specified by the "mem-path" option). Therefore,
>> it's not necessary in such cases to specify the file size via the
>> "size" option. This patch series makes the "size" option of
>> memory-backend-file optional. In detail,
>>
>> 1. if the size of the backend file is not zero and the "size" option
>>    is not present, QEMU will use the file size as the "size" option;
>>
>> 2. if the backend file is a non-existing file, an empty file, or a
>>    directory, the "size" option is still needed;
>>
>> 3. the "size" option is also needed in cases that QEMU fails to the
>>    backend file size.
>
>"fails to the backend file size": are you missing a verb?
>

Yes, it should be "fails to _get_ the backend file size".

Thanks,
Haozhong

>> Patch 1 is not quite relevant. It adds missing comments on errors
>> of get_file_size() per Eduardo's suggestion.
>>
>> Patch 2 moves the file operations from file_ram_alloc() to
>> qemu_ram_alloc_from_file(), so that all checks on the file size and
>> the "size" option, and the related operations on ram block length can
>> be put in qemu_ram_alloc_from_file() and no duplication is needed.
>>
>> Patch 3 actually implements the logic to make the option "size" optional.
>>
>> [1] https://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg06823.html
>> [2] https://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg00251.html
>>
>> Haozhong Zhang (3):
>>   exec.c: add comment for errors of get_file_size()
>>   exec.c: move file operations to qemu_ram_alloc_from_file()
>>   hostmem-file: make option 'size' optional
>>
>>  backends/hostmem-file.c |  28 ++++--
>>  exec.c                  | 242 ++++++++++++++++++++++++++++++------------------
>>  2 files changed, 170 insertions(+), 100 deletions(-)