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

[Hu Tao]

On Tue, Sep 02, 2014 at 11:45:38PM +0200, Max Reitz wrote:
> On 29.08.2014 10:33, 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 | 92 
> > +++++++++++++++++++++++++++++++++++++++++++------------
> >  qemu-doc.texi     |  8 +++++
> >  qemu-img.texi     |  8 +++++
> >  3 files changed, 88 insertions(+), 20 deletions(-)
> >
> >diff --git a/block/raw-posix.c b/block/raw-posix.c
> >index abe0759..25f66f2 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>
> >@@ -1365,6 +1366,9 @@ static int raw_create(const char *filename, QemuOpts 
> >*opts, Error **errp)
> >      int result = 0;
> >      int64_t total_size = 0;
> >      bool nocow = false;
> >+    PreallocMode prealloc = PREALLOC_MODE_OFF;
> >+    char *buf = NULL;
> >+    Error *local_err = NULL;
> >      strstart(filename, "file:", &filename);
> >@@ -1372,37 +1376,80 @@ static int raw_create(const char *filename, QemuOpts 
> >*opts, Error **errp)
> >      total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> >                            BDRV_SECTOR_SIZE);
> >      nocow = qemu_opt_get_bool(opts, BLOCK_OPT_NOCOW, false);
> >+    buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> >+    prealloc = qapi_enum_parse(PreallocMode_lookup, buf,
> >+                               PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
> >+                               &local_err);
> >+    g_free(buf);
> >+    if (local_err) {
> >+        error_propagate(errp, local_err);
> >+        result = -EINVAL;
> >+        goto out;
> >+    }
> >      fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> >                     0644);
> >      if (fd < 0) {
> >          result = -errno;
> >          error_setg_errno(errp, -result, "Could not create file");
> >-    } else {
> >-        if (nocow) {
> >+        goto out;
> >+    }
> >+
> >+    if (nocow) {
> >  #ifdef __linux__
> >-            /* Set NOCOW flag to solve performance issue on fs like btrfs.
> >-             * This is an optimisation. The FS_IOC_SETFLAGS ioctl return 
> >value
> >-             * will be ignored since any failure of this operation should 
> >not
> >-             * block the left work.
> >-             */
> >-            int attr;
> >-            if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) {
> >-                attr |= FS_NOCOW_FL;
> >-                ioctl(fd, FS_IOC_SETFLAGS, &attr);
> >-            }
> >-#endif
> >+        /* Set NOCOW flag to solve performance issue on fs like btrfs.
> >+         * This is an optimisation. The FS_IOC_SETFLAGS ioctl return value
> >+         * will be ignored since any failure of this operation should not
> >+         * block the left work.
> >+         */
> >+        int attr;
> >+        if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) {
> >+            attr |= FS_NOCOW_FL;
> >+            ioctl(fd, FS_IOC_SETFLAGS, &attr);
> >          }
> >+#endif
> >+    }
> >-        if (ftruncate(fd, total_size) != 0) {
> >-            result = -errno;
> >-            error_setg_errno(errp, -result, "Could not resize file");
> >-        }
> >-        if (qemu_close(fd) != 0) {
> >-            result = -errno;
> >-            error_setg_errno(errp, -result, "Could not close the new file");
> >+    if (ftruncate(fd, total_size) != 0) {
> >+        result = -errno;
> >+        error_setg_errno(errp, -result, "Could not resize file");
> >+        goto out_close;
> >+    }
> >+
> >+    if (prealloc == PREALLOC_MODE_FULL) {
> >+        /* posix_fallocate() doesn't set errno. */
> >+        result = -posix_fallocate(fd, 0, total_size);
> >+        if (result != 0) {
> >+            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;
> >+            }
> >+            fsync(fd);
> >+            g_free(buf);
> >          }
> >+    } else if (prealloc != PREALLOC_MODE_OFF) {
> >+        result = -1;
> 
> As for qcow2 in patch 4, I'd prefer -EINVAL.

Okay.

> 
> >+        error_setg(errp, "Unsupported preallocation mode: %s",
> >+                   PreallocMode_lookup[prealloc]);
> >+    }
> >+
> >+out_close:
> >+    if (qemu_close(fd) != 0 && result == 0) {
> >+        result = -errno;
> >+        error_setg_errno(errp, -result, "Could not close the new file");
> >      }
> >+out:
> >      return result;
> >  }
> >@@ -1585,6 +1632,11 @@ static QemuOptsList raw_create_opts = {
> >              .type = QEMU_OPT_BOOL,
> >              .help = "Turn off copy-on-write (valid only on btrfs)"
> >          },
> >+        {
> >+            .name = BLOCK_OPT_PREALLOC,
> >+            .type = QEMU_OPT_STRING,
> >+            .help = "Preallocation mode (allowed values: off, full)"
> >+        },
> >          { /* end of list */ }
> >      }
> >  };
> >diff --git a/qemu-doc.texi b/qemu-doc.texi
> >index 2b232ae..2637765 100644
> >--- a/qemu-doc.texi
> >+++ b/qemu-doc.texi
> >@@ -527,6 +527,14 @@ Linux or NTFS on Windows), then only the written 
> >sectors will reserve
> >  space. Use @code{qemu-img info} to know the real size used by the
> >  image or @code{ls -ls} on Unix/Linux.
> >+Supported options:
> >address@hidden @code
> >address@hidden preallocation
> >+Preallocation mode(allowed values: @code{off}, @code{full}). An image is
> 
> Missing space in front of the opening bracket.

Okay. I checked that in other places of opening bracket there is a space
before them.

> 
> >+fully preallocated by calling posix_fallocate() if it's available, or by
> 
> Hm, I personally am not so happy about contractions ("it's") in
> persistent documentation (even source code comments). Although I
> know there are already some of them in qemu-doc.texi, I'd rather
> avoid them. But I'll leave this up to you as I'm no native speaker.

I'm not, either. I don't know which one in "it's" and "it is" is more
common, but I can change the contraction if it makes you feel better:)

Regards,
Hu

> 
> >+writing zeros to underlying storage.
> >address@hidden table
> >+
> >  @item qcow2
> >  QEMU image format, the most versatile format. Use it to have smaller
> >  images (useful if your filesystem does not supports holes, for example
> >diff --git a/qemu-img.texi b/qemu-img.texi
> >index cb68948..063ec61 100644
> >--- a/qemu-img.texi
> >+++ b/qemu-img.texi
> >@@ -418,6 +418,14 @@ Linux or NTFS on Windows), then only the written 
> >sectors will reserve
> >  space. Use @code{qemu-img info} to know the real size used by the
> >  image or @code{ls -ls} on Unix/Linux.
> >+Supported options:
> >address@hidden @code
> >address@hidden preallocation
> >+Preallocation mode(allowed values: @code{off}, @code{full}). An image is
> >+fully preallocated by calling posix_fallocate() if it's available, or by
> >+writing zeros to underlying storage.
> >address@hidden table
> >+
> 
> Same as for qemu-doc.texi.
> 
> However, these are all minor, so with "result = -EINVAL" and the
> missing space added before the brackets:
> 
> Reviewed-by: Max Reitz <address@hidden>
> 
> >  @item qcow2
> >  QEMU image format, the most versatile format. Use it to have smaller
> >  images (useful if your filesystem does not supports holes, for example