Re: [Qemu-devel] [PATCH v10 5/6] raw-posix: Add full image preallocation option

[Hu Tao]

On Sat, Jun 14, 2014 at 09:38:30PM +0200, Max Reitz wrote:
> On 12.06.2014 05:54, Hu Tao wrote:
> >This patch adds a new option preallocation for raw format, and implements
> >full preallocation.
> >
> >Signed-off-by: Hu Tao <address@hidden>
> >---
> >  block/raw-posix.c | 59 
> > ++++++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 52 insertions(+), 7 deletions(-)
> >
> >diff --git a/block/raw-posix.c b/block/raw-posix.c
> >index 35057f0..73b10cd 100644
> >--- a/block/raw-posix.c
> >+++ b/block/raw-posix.c
> >@@ -30,6 +30,7 @@
> >  #include "block/thread-pool.h"
> >  #include "qemu/iov.h"
> >  #include "raw-aio.h"
> >+#include "qapi/util.h"
> >  #if defined(__APPLE__) && (__MACH__)
> >  #include <paths.h>
> >@@ -1279,6 +1280,8 @@ static int raw_create(const char *filename, 
> >QEMUOptionParameter *options,
> >      int fd;
> >      int result = 0;
> >      int64_t total_size = 0;
> >+    PreallocMode prealloc = PREALLOC_MODE_OFF;
> >+    Error *error = NULL;
> >      strstart(filename, "file:", &filename);
> >@@ -1286,6 +1289,14 @@ static int raw_create(const char *filename, 
> >QEMUOptionParameter *options,
> >      while (options && options->name) {
> >          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> >              total_size = ROUND_UP(options->value.n, BDRV_SECTOR_SIZE);
> >+        } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
> >+            prealloc = qapi_enum_parse(PreallocMode_lookup, 
> >options->value.s,
> >+                                       PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
> >+                                       &error);
> >+            if (error) {
> >+                error_propagate(errp, error);
> >+                return -EINVAL;
> 
> Could be "result = -EINVAL; goto out;" instead, but definitely isn't
> wrong this way.
> 
> >+            }
> >          }
> >          options++;
> >      }
> >@@ -1295,16 +1306,45 @@ static int raw_create(const char *filename, 
> >QEMUOptionParameter *options,
> >      if (fd < 0) {
> >          result = -errno;
> >          error_setg_errno(errp, -result, "Could not create file");
> >-    } else {
> >-        if (ftruncate(fd, total_size) != 0) {
> >-            result = -errno;
> >-            error_setg_errno(errp, -result, "Could not resize file");
> >+        goto out;
> >+    }
> >+    if (ftruncate(fd, total_size) != 0) {
> >+        result = -errno;
> >+        error_setg_errno(errp, -result, "Could not resize file");
> >+        goto out_close;
> >+    }
> >+    if (prealloc == PREALLOC_MODE_METADATA) {
> >+        /* posix_fallocate() doesn't set errno. */
> >+        result = -posix_fallocate(fd, 0, total_size);
> >+        if (result != 0) {
> >+            error_setg_errno(errp, -result,
> >+                             "Could not preallocate data for the new file");
> >          }
> >-        if (qemu_close(fd) != 0) {
> >-            result = -errno;
> >-            error_setg_errno(errp, -result, "Could not close the new file");
> >+    } else if (prealloc == PREALLOC_MODE_FULL) {
> >+        char *buf = g_malloc0(65536);
> >+        int64_t num = 0, left = total_size;
> >+
> >+        while (left > 0) {
> >+            num = MIN(left, 65536);
> >+            result = write(fd, buf, num);
> >+            if (result < 0) {
> >+                result = -errno;
> >+                error_setg_errno(errp, -result,
> >+                                 "Could not write to the new file");
> >+                g_free(buf);
> >+                goto out_close;
> >+            }
> >+            left -= num;
> >          }
> 
> Technically, a raw file does not have any metadata, therefore
> preallocation=metadata is a bit ambiguous. I'd accept both
> allocating nothing and allocating everything; you chose the latter,
> which is fine.

qcow2's behaviour of metadata preallocation depends on raw's, this is
why I chose the latter here.

> 
> However, why do you implement the preallocation differently for
> preallocation=full, then? posix_fallocate() does not seem to change
> the contents of the areas which were newly allocated; and the
> ftruncate() before made sure they are read back as zeroes.
> Therefore, using ftruncate() and then posix_fallocate() seems to be
> functionally equivalent to writing zeroes.

The difference is posix_fallocate() reserves space but ftruncate()
doesn't, as said in man page:

  After a successful call to posix_fallocate(), subsequent writes
  to bytes in the specified range are guaranteed not to fail because
  of lack of disk space.

however, posix_fallocate() doesn't guarantee this in other cases like
thin provisioning, this is why preallocation=metadata is different than
preallocation=full.

Hu