[Qemu-devel] [PATCH v5 RESEND 0/4] qemu-img: add preallocation=full

[Qemu-devel] [PATCH v5 RESEND 0/4] qemu-img: add preallocation=full

[Hu Tao]

This series implements full image preallocation to create a non-sparse image
file at creation time, both for raw and qcow2 format. The purpose is to avoid
performance deterioration of the guest cause by sparse image.

v5:
  - fix wrong calculation of qcow2 metadata size in v4
  - remove raw_preallocate2()
  - better error out path in raw_create()
  - fix coding style


Hu Tao (4):
  qapi: introduce PreallocMode and a new PreallocMode full.
  raw, qcow2: don't convert file size to sector size
  raw-posix: Add full image preallocation option
  qcow2: Add full image preallocation option

 block/qcow2.c     | 91 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 block/raw-posix.c | 45 +++++++++++++++++++++------
 qapi-schema.json  | 12 ++++++++
 3 files changed, 128 insertions(+), 20 deletions(-)

-- 
1.8.0

[Qemu-devel] [PATCH v5 RESEND 1/4] qapi: introduce PreallocMode and a new PreallocMode full.

[Hu Tao]

This patch prepares for the subsequent patches.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2.c    |  8 ++++----
 qapi-schema.json | 12 ++++++++++++
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 99a1ad1..30e36bc 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1452,7 +1452,7 @@ static int preallocate(BlockDriverState *bs)
 
 static int qcow2_create2(const char *filename, int64_t total_size,
                          const char *backing_file, const char *backing_format,
-                         int flags, size_t cluster_size, int prealloc,
+                         int flags, size_t cluster_size, PreallocMode prealloc,
                          QEMUOptionParameter *options, int version,
                          Error **errp)
 {
@@ -1622,7 +1622,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
     uint64_t sectors = 0;
     int flags = 0;
     size_t cluster_size = DEFAULT_CLUSTER_SIZE;
-    int prealloc = 0;
+    PreallocMode prealloc = PREALLOC_MODE_OFF;
     int version = 3;
     Error *local_err = NULL;
     int ret;
@@ -1643,9 +1643,9 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
             }
         } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
             if (!options->value.s || !strcmp(options->value.s, "off")) {
-                prealloc = 0;
+                prealloc = PREALLOC_MODE_OFF;
             } else if (!strcmp(options->value.s, "metadata")) {
-                prealloc = 1;
+                prealloc = PREALLOC_MODE_METADATA;
             } else {
                 error_setg(errp, "Invalid preallocation mode: '%s'",
                            options->value.s);
diff --git a/qapi-schema.json b/qapi-schema.json
index 05ced9d..f86068c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4419,3 +4419,15 @@
 # Since: 1.7
 ##
 { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
+
+##
+# @PreallocMode
+#
+# Preallocation mode of QEMU image file
+#
+# @off: no preallocation
+# @metadata: preallocate only for metadata
+# @full: preallocate all data, including metadata
+##
+{ 'enum': 'PreallocMode',
+  'data': [ 'off', 'metadata', 'full' ] }
-- 
1.8.0

[Qemu-devel] [PATCH v5 RESEND 2/4] raw, qcow2: don't convert file size to sector size

[Hu Tao]

and avoid convert it back later.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2.c     | 8 ++++----
 block/raw-posix.c | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 30e36bc..e4bab70 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1569,7 +1569,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     }
 
     /* Okay, now that we have a valid image, let's give it the right size */
-    ret = bdrv_truncate(bs, total_size * BDRV_SECTOR_SIZE);
+    ret = bdrv_truncate(bs, total_size);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not resize image");
         goto out;
@@ -1619,7 +1619,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
 {
     const char *backing_file = NULL;
     const char *backing_fmt = NULL;
-    uint64_t sectors = 0;
+    uint64_t size = 0;
     int flags = 0;
     size_t cluster_size = DEFAULT_CLUSTER_SIZE;
     PreallocMode prealloc = PREALLOC_MODE_OFF;
@@ -1630,7 +1630,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
     /* Read out options */
     while (options && options->name) {
         if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
-            sectors = options->value.n / 512;
+            size = options->value.n & BDRV_SECTOR_MASK;
         } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
             backing_file = options->value.s;
         } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FMT)) {
@@ -1681,7 +1681,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
         return -EINVAL;
     }
 
-    ret = qcow2_create2(filename, sectors, backing_file, backing_fmt, flags,
+    ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
                         cluster_size, prealloc, options, version, &local_err);
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 126a634..01fb41a 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1233,7 +1233,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
     /* Read out options */
     while (options && options->name) {
         if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
-            total_size = options->value.n / BDRV_SECTOR_SIZE;
+            total_size = options->value.n & BDRV_SECTOR_MASK;
         }
         options++;
     }
@@ -1244,7 +1244,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
         result = -errno;
         error_setg_errno(errp, -result, "Could not create file");
     } else {
-        if (ftruncate(fd, total_size * BDRV_SECTOR_SIZE) != 0) {
+        if (ftruncate(fd, total_size) != 0) {
             result = -errno;
             error_setg_errno(errp, -result, "Could not resize file");
         }
-- 
1.8.0

[Qemu-devel] [PATCH v5 RESEND 3/4] raw-posix: Add full image preallocation option

[Hu Tao]

This patch adds a new option preallocation for raw format, and implements
full preallocation.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/raw-posix.c | 43 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 01fb41a..1961b74 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1229,11 +1229,22 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
     int fd;
     int result = 0;
     int64_t total_size = 0;
+    PreallocMode prealloc = PREALLOC_MODE_OFF;
 
     /* Read out options */
     while (options && options->name) {
         if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
             total_size = options->value.n & BDRV_SECTOR_MASK;
+        } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
+            if (!options->value.s || !strcmp(options->value.s, "off")) {
+                prealloc = PREALLOC_MODE_OFF;
+            } else if (!strcmp(options->value.s, "full")) {
+                prealloc = PREALLOC_MODE_FULL;
+            } else {
+                error_setg(errp, "Invalid preallocation mode: '%s'",
+                           options->value.s);
+                return -EINVAL;
+            }
         }
         options++;
     }
@@ -1243,16 +1254,27 @@ 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");
-        }
-        if (qemu_close(fd) != 0) {
-            result = -errno;
-            error_setg_errno(errp, -result, "Could not close the new 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_FULL) {
+        /* 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");
         }
     }
+out_close:
+    if (qemu_close(fd) != 0) {
+        result = -errno;
+        error_setg_errno(errp, -result, "Could not close the new file");
+    }
+out:
     return result;
 }
 
@@ -1403,6 +1425,11 @@ static QEMUOptionParameter raw_create_options[] = {
         .type = OPT_SIZE,
         .help = "Virtual disk size"
     },
+    {
+        .name = BLOCK_OPT_PREALLOC,
+        .type = OPT_STRING,
+        .help = "Preallocation mode (allowed values: off, full)"
+    },
     { NULL }
 };
 
-- 
1.8.0

[Qemu-devel] [PATCH v5 RESEND 4/4] qcow2: Add full image preallocation option

[Hu Tao]

This adds a preallocation=full mode to qcow2 image creation, which
creates a non-sparse image file.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 72 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index e4bab70..4b113b7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1456,6 +1456,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
                          QEMUOptionParameter *options, int version,
                          Error **errp)
 {
+    QEMUOptionParameter *alloc_options = NULL;
     /* Calculate cluster_bits */
     int cluster_bits;
     cluster_bits = ffs(cluster_size) - 1;
@@ -1485,16 +1486,80 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     Error *local_err = NULL;
     int ret;
 
+    if (prealloc == PREALLOC_MODE_FULL) {
+        int64_t meta_size = 0;
+        unsigned nreftablee, nrefblocke, nl1e, nl2e;
+        BlockDriver *drv;
+
+        total_size = align_offset(total_size, cluster_size);
+
+        drv = bdrv_find_protocol(filename, true);
+        if (drv == NULL) {
+            error_setg(errp, "Could not find protocol for file '%s'", filename);
+            return -ENOENT;
+        }
+
+        alloc_options = append_option_parameters(alloc_options,
+                                                 drv->create_options);
+        alloc_options = append_option_parameters(alloc_options, options);
+
+        /* header: 1 cluster */
+        meta_size += cluster_size;
+
+        /* total size of L2 tables */
+        nl2e = total_size / cluster_size;
+        nl2e = align_offset(nl2e, cluster_size / sizeof(uint64_t));
+        meta_size += nl2e * sizeof(uint64_t);
+
+        /* total size of L1 tables */
+        nl1e = nl2e * sizeof(uint64_t) / cluster_size;
+        nl1e = align_offset(nl1e, cluster_size / sizeof(uint64_t));
+        meta_size += nl1e * sizeof(uint64_t);
+
+        /* total size of refcount blocks
+         *
+         * note: every host cluster is reference-counted, including metadata
+         * (even refcount blocks are recursively included).
+         * Let:
+         *   a = total_size (this is the guest disk size)
+         *   m = meta size not including refcount blocks and refcount tables
+         *   c = cluster size
+         *   y1 = number of refcount blocks entries
+         *   y2 = meta size including everything
+         * then,
+         *   y1 = (y2 + a)/c
+         *   y2 = y1 * sizeof(u16) + y1 * sizeof(u16) * sizeof(u64) / c + m
+         * we can get y1:
+         *   y1 = (a + m) / (c - sizeof(u16) - sizeof(u16) * sizeof(u64) / c)
+         */
+        nrefblocke = (total_size + meta_size + cluster_size) /
+            (cluster_size - sizeof(uint16_t) -
+             1.0 * sizeof(uint16_t) * sizeof(uint64_t) / cluster_size);
+        nrefblocke = align_offset(nrefblocke, cluster_size / sizeof(uint16_t));
+        meta_size += nrefblocke * sizeof(uint16_t);
+
+        /* total size of refcount tables */
+        nreftablee = nrefblocke * sizeof(uint16_t) / cluster_size;
+        nreftablee = align_offset(nreftablee, cluster_size / sizeof(uint64_t));
+        meta_size += nreftablee * sizeof(uint64_t);
+
+        set_option_parameter_int(alloc_options, BLOCK_OPT_SIZE,
+                                 total_size + meta_size);
+        set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC, "full");
+
+        options = alloc_options;
+    }
+
     ret = bdrv_create_file(filename, options, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
-        return ret;
+        goto out_options;
     }
 
     ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
-        return ret;
+        goto out_options;
     }
 
     /* Write the header */
@@ -1611,6 +1676,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     ret = 0;
 out:
     bdrv_unref(bs);
+out_options:
+    free_option_parameters(alloc_options);
     return ret;
 }
 
@@ -1646,6 +1713,8 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
                 prealloc = PREALLOC_MODE_OFF;
             } else if (!strcmp(options->value.s, "metadata")) {
                 prealloc = PREALLOC_MODE_METADATA;
+            } else if (!strcmp(options->value.s, "full")) {
+                prealloc = PREALLOC_MODE_FULL;
             } else {
                 error_setg(errp, "Invalid preallocation mode: '%s'",
                            options->value.s);
@@ -2211,7 +2280,7 @@ static QEMUOptionParameter qcow2_create_options[] = {
     {
         .name = BLOCK_OPT_PREALLOC,
         .type = OPT_STRING,
-        .help = "Preallocation mode (allowed values: off, metadata)"
+        .help = "Preallocation mode (allowed values: off, metadata, full)"
     },
     {
         .name = BLOCK_OPT_LAZY_REFCOUNTS,
-- 
1.8.0

Re: [Qemu-devel] [PATCH v5 RESEND 2/4] raw, qcow2: don't convert file size to sector size

[Fam Zheng]

On Tue, 02/11 15:07, Hu Tao wrote:
> and avoid convert it back later.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  block/qcow2.c     | 8 ++++----
>  block/raw-posix.c | 4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 30e36bc..e4bab70 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1569,7 +1569,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>      }
>  
>      /* Okay, now that we have a valid image, let's give it the right size */
> -    ret = bdrv_truncate(bs, total_size * BDRV_SECTOR_SIZE);
> +    ret = bdrv_truncate(bs, total_size);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Could not resize image");
>          goto out;
> @@ -1619,7 +1619,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
>  {
>      const char *backing_file = NULL;
>      const char *backing_fmt = NULL;
> -    uint64_t sectors = 0;
> +    uint64_t size = 0;
>      int flags = 0;
>      size_t cluster_size = DEFAULT_CLUSTER_SIZE;
>      PreallocMode prealloc = PREALLOC_MODE_OFF;
> @@ -1630,7 +1630,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
>      /* Read out options */
>      while (options && options->name) {
>          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> -            sectors = options->value.n / 512;
> +            size = options->value.n & BDRV_SECTOR_MASK;
>          } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
>              backing_file = options->value.s;
>          } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FMT)) {
> @@ -1681,7 +1681,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
>          return -EINVAL;
>      }
>  
> -    ret = qcow2_create2(filename, sectors, backing_file, backing_fmt, flags,
> +    ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
>                          cluster_size, prealloc, options, version, &local_err);
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 126a634..01fb41a 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1233,7 +1233,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
>      /* Read out options */
>      while (options && options->name) {
>          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> -            total_size = options->value.n / BDRV_SECTOR_SIZE;
> +            total_size = options->value.n & BDRV_SECTOR_MASK;
>          }
>          options++;
>      }
> @@ -1244,7 +1244,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
>          result = -errno;
>          error_setg_errno(errp, -result, "Could not create file");
>      } else {
> -        if (ftruncate(fd, total_size * BDRV_SECTOR_SIZE) != 0) {
> +        if (ftruncate(fd, total_size) != 0) {
>              result = -errno;
>              error_setg_errno(errp, -result, "Could not resize file");
>          }
> -- 
> 1.8.0
> 

Why not change raw-win32.c as well? Otherwise it will be confusing when
total_size means differently in two counterpart files.

Thanks,
Fam

Re: [Qemu-devel] [PATCH v5 RESEND 1/4] qapi: introduce PreallocMode and a new PreallocMode full.

[Fam Zheng]

On Tue, 02/11 15:07, Hu Tao wrote:
> This patch prepares for the subsequent patches.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  block/qcow2.c    |  8 ++++----
>  qapi-schema.json | 12 ++++++++++++
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 99a1ad1..30e36bc 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1452,7 +1452,7 @@ static int preallocate(BlockDriverState *bs)
>  
>  static int qcow2_create2(const char *filename, int64_t total_size,
>                           const char *backing_file, const char *backing_format,
> -                         int flags, size_t cluster_size, int prealloc,
> +                         int flags, size_t cluster_size, PreallocMode prealloc,
>                           QEMUOptionParameter *options, int version,
>                           Error **errp)
>  {
> @@ -1622,7 +1622,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
>      uint64_t sectors = 0;
>      int flags = 0;
>      size_t cluster_size = DEFAULT_CLUSTER_SIZE;
> -    int prealloc = 0;
> +    PreallocMode prealloc = PREALLOC_MODE_OFF;
>      int version = 3;
>      Error *local_err = NULL;
>      int ret;
> @@ -1643,9 +1643,9 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
>              }
>          } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
>              if (!options->value.s || !strcmp(options->value.s, "off")) {
> -                prealloc = 0;
> +                prealloc = PREALLOC_MODE_OFF;
>              } else if (!strcmp(options->value.s, "metadata")) {
> -                prealloc = 1;
> +                prealloc = PREALLOC_MODE_METADATA;
>              } else {
>                  error_setg(errp, "Invalid preallocation mode: '%s'",
>                             options->value.s);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 05ced9d..f86068c 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4419,3 +4419,15 @@
>  # Since: 1.7
>  ##
>  { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> +
> +##
> +# @PreallocMode
> +#
> +# Preallocation mode of QEMU image file
> +#
> +# @off: no preallocation
> +# @metadata: preallocate only for metadata
> +# @full: preallocate all data, including metadata
> +##
> +{ 'enum': 'PreallocMode',
> +  'data': [ 'off', 'metadata', 'full' ] }
> -- 
> 1.8.0
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v5 RESEND 3/4] raw-posix: Add full image preallocation option

[Fam Zheng]

On Tue, 02/11 15:07, Hu Tao wrote:
> This patch adds a new option preallocation for raw format, and implements
> full preallocation.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  block/raw-posix.c | 43 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 01fb41a..1961b74 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1229,11 +1229,22 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
>      int fd;
>      int result = 0;
>      int64_t total_size = 0;
> +    PreallocMode prealloc = PREALLOC_MODE_OFF;
>  
>      /* Read out options */
>      while (options && options->name) {
>          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
>              total_size = options->value.n & BDRV_SECTOR_MASK;
> +        } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
> +            if (!options->value.s || !strcmp(options->value.s, "off")) {
> +                prealloc = PREALLOC_MODE_OFF;
> +            } else if (!strcmp(options->value.s, "full")) {
> +                prealloc = PREALLOC_MODE_FULL;
> +            } else {
> +                error_setg(errp, "Invalid preallocation mode: '%s'",
> +                           options->value.s);
> +                return -EINVAL;
> +            }
>          }
>          options++;
>      }
> @@ -1243,16 +1254,27 @@ 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");
> -        }
> -        if (qemu_close(fd) != 0) {
> -            result = -errno;
> -            error_setg_errno(errp, -result, "Could not close the new 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_FULL) {
> +        /* 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");
>          }
>      }
> +out_close:
> +    if (qemu_close(fd) != 0) {
> +        result = -errno;
> +        error_setg_errno(errp, -result, "Could not close the new file");

If errp is already set because ftruncate or posix_ftruncate failed, and
qemu_close() fails too, the call to error_setg_errno() will abort on failing
the assertion of (*errp == NULL).

You could either embed the two failures in one error message, or pick the
urgent one and drop the other.

Thanks,
Fam

> +    }
> +out:
>      return result;
>  }
>  
> @@ -1403,6 +1425,11 @@ static QEMUOptionParameter raw_create_options[] = {
>          .type = OPT_SIZE,
>          .help = "Virtual disk size"
>      },
> +    {
> +        .name = BLOCK_OPT_PREALLOC,
> +        .type = OPT_STRING,
> +        .help = "Preallocation mode (allowed values: off, full)"
> +    },
>      { NULL }
>  };
>  
> -- 
> 1.8.0
> 

Re: [Qemu-devel] [PATCH v5 RESEND 1/4] qapi: introduce PreallocMode and a new PreallocMode full.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 959 bytes --]

On 02/11/2014 12:07 AM, Hu Tao wrote:
> This patch prepares for the subsequent patches.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  block/qcow2.c    |  8 ++++----
>  qapi-schema.json | 12 ++++++++++++
>  2 files changed, 16 insertions(+), 4 deletions(-)

> +++ b/qapi-schema.json
> @@ -4419,3 +4419,15 @@
>  # Since: 1.7
>  ##
>  { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> +
> +##
> +# @PreallocMode
> +#
> +# Preallocation mode of QEMU image file
> +#
> +# @off: no preallocation
> +# @metadata: preallocate only for metadata
> +# @full: preallocate all data, including metadata

Missing a line:
# Since 2.0

> +##
> +{ 'enum': 'PreallocMode',
> +  'data': [ 'off', 'metadata', 'full' ] }
> 

With that fixed, you can add:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v5 RESEND 2/4] raw, qcow2: don't convert file size to sector size

[Hu Tao]

On Tue, Feb 11, 2014 at 04:53:30PM +0800, Fam Zheng wrote:
> On Tue, 02/11 15:07, Hu Tao wrote:
> > and avoid convert it back later.
> > 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  block/qcow2.c     | 8 ++++----
> >  block/raw-posix.c | 4 ++--
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 30e36bc..e4bab70 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -1569,7 +1569,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
> >      }
> >  
> >      /* Okay, now that we have a valid image, let's give it the right size */
> > -    ret = bdrv_truncate(bs, total_size * BDRV_SECTOR_SIZE);
> > +    ret = bdrv_truncate(bs, total_size);
> >      if (ret < 0) {
> >          error_setg_errno(errp, -ret, "Could not resize image");
> >          goto out;
> > @@ -1619,7 +1619,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
> >  {
> >      const char *backing_file = NULL;
> >      const char *backing_fmt = NULL;
> > -    uint64_t sectors = 0;
> > +    uint64_t size = 0;
> >      int flags = 0;
> >      size_t cluster_size = DEFAULT_CLUSTER_SIZE;
> >      PreallocMode prealloc = PREALLOC_MODE_OFF;
> > @@ -1630,7 +1630,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
> >      /* Read out options */
> >      while (options && options->name) {
> >          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> > -            sectors = options->value.n / 512;
> > +            size = options->value.n & BDRV_SECTOR_MASK;
> >          } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> >              backing_file = options->value.s;
> >          } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FMT)) {
> > @@ -1681,7 +1681,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
> >          return -EINVAL;
> >      }
> >  
> > -    ret = qcow2_create2(filename, sectors, backing_file, backing_fmt, flags,
> > +    ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
> >                          cluster_size, prealloc, options, version, &local_err);
> >      if (error_is_set(&local_err)) {
> >          error_propagate(errp, local_err);
> > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > index 126a634..01fb41a 100644
> > --- a/block/raw-posix.c
> > +++ b/block/raw-posix.c
> > @@ -1233,7 +1233,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
> >      /* Read out options */
> >      while (options && options->name) {
> >          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> > -            total_size = options->value.n / BDRV_SECTOR_SIZE;
> > +            total_size = options->value.n & BDRV_SECTOR_MASK;
> >          }
> >          options++;
> >      }
> > @@ -1244,7 +1244,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
> >          result = -errno;
> >          error_setg_errno(errp, -result, "Could not create file");
> >      } else {
> > -        if (ftruncate(fd, total_size * BDRV_SECTOR_SIZE) != 0) {
> > +        if (ftruncate(fd, total_size) != 0) {
> >              result = -errno;
> >              error_setg_errno(errp, -result, "Could not resize file");
> >          }
> > -- 
> > 1.8.0
> > 
> 
> Why not change raw-win32.c as well? Otherwise it will be confusing when
> total_size means differently in two counterpart files.

Makes sense to make it consistent in all places.

> 
> Thanks,
> Fam

Re: [Qemu-devel] [PATCH v5 RESEND 1/4] qapi: introduce PreallocMode and a new PreallocMode full.

[Hu Tao]

On Tue, Feb 11, 2014 at 06:20:03AM -0700, Eric Blake wrote:
> On 02/11/2014 12:07 AM, Hu Tao wrote:
> > This patch prepares for the subsequent patches.
> > 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  block/qcow2.c    |  8 ++++----
> >  qapi-schema.json | 12 ++++++++++++
> >  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> > +++ b/qapi-schema.json
> > @@ -4419,3 +4419,15 @@
> >  # Since: 1.7
> >  ##
> >  { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> > +
> > +##
> > +# @PreallocMode
> > +#
> > +# Preallocation mode of QEMU image file
> > +#
> > +# @off: no preallocation
> > +# @metadata: preallocate only for metadata
> > +# @full: preallocate all data, including metadata
> 
> Missing a line:
> # Since 2.0

Thanks.

Re: [Qemu-devel] [PATCH v5 RESEND 3/4] raw-posix: Add full image preallocation option

[Hu Tao]

On Tue, Feb 11, 2014 at 05:04:09PM +0800, Fam Zheng wrote:
> On Tue, 02/11 15:07, Hu Tao wrote:
> > This patch adds a new option preallocation for raw format, and implements
> > full preallocation.
> > 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  block/raw-posix.c | 43 +++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 35 insertions(+), 8 deletions(-)
> > 
> > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > index 01fb41a..1961b74 100644
> > --- a/block/raw-posix.c
> > +++ b/block/raw-posix.c
> > @@ -1229,11 +1229,22 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
> >      int fd;
> >      int result = 0;
> >      int64_t total_size = 0;
> > +    PreallocMode prealloc = PREALLOC_MODE_OFF;
> >  
> >      /* Read out options */
> >      while (options && options->name) {
> >          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> >              total_size = options->value.n & BDRV_SECTOR_MASK;
> > +        } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
> > +            if (!options->value.s || !strcmp(options->value.s, "off")) {
> > +                prealloc = PREALLOC_MODE_OFF;
> > +            } else if (!strcmp(options->value.s, "full")) {
> > +                prealloc = PREALLOC_MODE_FULL;
> > +            } else {
> > +                error_setg(errp, "Invalid preallocation mode: '%s'",
> > +                           options->value.s);
> > +                return -EINVAL;
> > +            }
> >          }
> >          options++;
> >      }
> > @@ -1243,16 +1254,27 @@ 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");
> > -        }
> > -        if (qemu_close(fd) != 0) {
> > -            result = -errno;
> > -            error_setg_errno(errp, -result, "Could not close the new 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_FULL) {
> > +        /* 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");
> >          }
> >      }
> > +out_close:
> > +    if (qemu_close(fd) != 0) {
> > +        result = -errno;
> > +        error_setg_errno(errp, -result, "Could not close the new file");
> 
> If errp is already set because ftruncate or posix_ftruncate failed, and
> qemu_close() fails too, the call to error_setg_errno() will abort on failing
> the assertion of (*errp == NULL).

The original code also has the problem. This brings a general problem
that two error_setg_errno() could be called on the same errp. In general
the first error is better to reveal to user.

> 
> You could either embed the two failures in one error message, or pick the
> urgent one and drop the other.
> 
> Thanks,
> Fam
> 
> > +    }
> > +out:
> >      return result;
> >  }
> >  
> > @@ -1403,6 +1425,11 @@ static QEMUOptionParameter raw_create_options[] = {
> >          .type = OPT_SIZE,
> >          .help = "Virtual disk size"
> >      },
> > +    {
> > +        .name = BLOCK_OPT_PREALLOC,
> > +        .type = OPT_STRING,
> > +        .help = "Preallocation mode (allowed values: off, full)"
> > +    },
> >      { NULL }
> >  };
> >  
> > -- 
> > 1.8.0
> > 

Re: [Qemu-devel] [PATCH v5 RESEND 4/4] qcow2: Add full image preallocation option

[Hu Tao]

On Tue, Feb 11, 2014 at 03:07:10PM +0800, Hu Tao wrote:
> This adds a preallocation=full mode to qcow2 image creation, which
> creates a non-sparse image file.

Ping.

This is the major part of the patchset that needs review, especially 
the calculation of metadata size.

> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  block/qcow2.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 72 insertions(+), 3 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index e4bab70..4b113b7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1456,6 +1456,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>                           QEMUOptionParameter *options, int version,
>                           Error **errp)
>  {
> +    QEMUOptionParameter *alloc_options = NULL;
>      /* Calculate cluster_bits */
>      int cluster_bits;
>      cluster_bits = ffs(cluster_size) - 1;
> @@ -1485,16 +1486,80 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>      Error *local_err = NULL;
>      int ret;
>  
> +    if (prealloc == PREALLOC_MODE_FULL) {
> +        int64_t meta_size = 0;
> +        unsigned nreftablee, nrefblocke, nl1e, nl2e;
> +        BlockDriver *drv;
> +
> +        total_size = align_offset(total_size, cluster_size);
> +
> +        drv = bdrv_find_protocol(filename, true);
> +        if (drv == NULL) {
> +            error_setg(errp, "Could not find protocol for file '%s'", filename);
> +            return -ENOENT;
> +        }
> +
> +        alloc_options = append_option_parameters(alloc_options,
> +                                                 drv->create_options);
> +        alloc_options = append_option_parameters(alloc_options, options);
> +
> +        /* header: 1 cluster */
> +        meta_size += cluster_size;
> +
> +        /* total size of L2 tables */
> +        nl2e = total_size / cluster_size;
> +        nl2e = align_offset(nl2e, cluster_size / sizeof(uint64_t));
> +        meta_size += nl2e * sizeof(uint64_t);
> +
> +        /* total size of L1 tables */
> +        nl1e = nl2e * sizeof(uint64_t) / cluster_size;
> +        nl1e = align_offset(nl1e, cluster_size / sizeof(uint64_t));

Although not stated in the qcow2 spec, but actually l1 tables are
aligned to cluster too because of the nature of qcow2_alloc_clusters().

Same for refcount tables below.

> +        meta_size += nl1e * sizeof(uint64_t);
> +
> +        /* total size of refcount blocks
> +         *
> +         * note: every host cluster is reference-counted, including metadata
> +         * (even refcount blocks are recursively included).
> +         * Let:
> +         *   a = total_size (this is the guest disk size)
> +         *   m = meta size not including refcount blocks and refcount tables
> +         *   c = cluster size
> +         *   y1 = number of refcount blocks entries
> +         *   y2 = meta size including everything
> +         * then,
> +         *   y1 = (y2 + a)/c
> +         *   y2 = y1 * sizeof(u16) + y1 * sizeof(u16) * sizeof(u64) / c + m
> +         * we can get y1:
> +         *   y1 = (a + m) / (c - sizeof(u16) - sizeof(u16) * sizeof(u64) / c)
> +         */
> +        nrefblocke = (total_size + meta_size + cluster_size) /
> +            (cluster_size - sizeof(uint16_t) -
> +             1.0 * sizeof(uint16_t) * sizeof(uint64_t) / cluster_size);
> +        nrefblocke = align_offset(nrefblocke, cluster_size / sizeof(uint16_t));
> +        meta_size += nrefblocke * sizeof(uint16_t);
> +
> +        /* total size of refcount tables */
> +        nreftablee = nrefblocke * sizeof(uint16_t) / cluster_size;
> +        nreftablee = align_offset(nreftablee, cluster_size / sizeof(uint64_t));
> +        meta_size += nreftablee * sizeof(uint64_t);
> +
> +        set_option_parameter_int(alloc_options, BLOCK_OPT_SIZE,
> +                                 total_size + meta_size);
> +        set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC, "full");
> +
> +        options = alloc_options;
> +    }
> +
>      ret = bdrv_create_file(filename, options, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
> -        return ret;
> +        goto out_options;
>      }
>  
>      ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
> -        return ret;
> +        goto out_options;
>      }
>  
>      /* Write the header */
> @@ -1611,6 +1676,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>      ret = 0;
>  out:
>      bdrv_unref(bs);
> +out_options:
> +    free_option_parameters(alloc_options);
>      return ret;
>  }
>  
> @@ -1646,6 +1713,8 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
>                  prealloc = PREALLOC_MODE_OFF;
>              } else if (!strcmp(options->value.s, "metadata")) {
>                  prealloc = PREALLOC_MODE_METADATA;
> +            } else if (!strcmp(options->value.s, "full")) {
> +                prealloc = PREALLOC_MODE_FULL;
>              } else {
>                  error_setg(errp, "Invalid preallocation mode: '%s'",
>                             options->value.s);
> @@ -2211,7 +2280,7 @@ static QEMUOptionParameter qcow2_create_options[] = {
>      {
>          .name = BLOCK_OPT_PREALLOC,
>          .type = OPT_STRING,
> -        .help = "Preallocation mode (allowed values: off, metadata)"
> +        .help = "Preallocation mode (allowed values: off, metadata, full)"
>      },
>      {
>          .name = BLOCK_OPT_LAZY_REFCOUNTS,
> -- 
> 1.8.0
> 

Re: [Qemu-devel] [PATCH v5 RESEND 0/4] qemu-img: add preallocation=full

[Stefan Hajnoczi]

On Tue, Feb 11, 2014 at 03:07:06PM +0800, Hu Tao wrote:
> This series implements full image preallocation to create a non-sparse image
> file at creation time, both for raw and qcow2 format. The purpose is to avoid
> performance deterioration of the guest cause by sparse image.
> 
> v5:
>   - fix wrong calculation of qcow2 metadata size in v4
>   - remove raw_preallocate2()
>   - better error out path in raw_create()
>   - fix coding style
> 
> 
> Hu Tao (4):
>   qapi: introduce PreallocMode and a new PreallocMode full.
>   raw, qcow2: don't convert file size to sector size
>   raw-posix: Add full image preallocation option
>   qcow2: Add full image preallocation option
> 
>  block/qcow2.c     | 91 ++++++++++++++++++++++++++++++++++++++++++++++++-------
>  block/raw-posix.c | 45 +++++++++++++++++++++------
>  qapi-schema.json  | 12 ++++++++
>  3 files changed, 128 insertions(+), 20 deletions(-)

Besides the comments that have been made, looks good.  The metadata size
calculation is correct now.

Re: [Qemu-devel] [PATCH v5 RESEND 0/4] qemu-img: add preallocation=full

[Hu Tao]

On Thu, Feb 27, 2014 at 03:09:57PM +0100, Stefan Hajnoczi wrote:
> On Tue, Feb 11, 2014 at 03:07:06PM +0800, Hu Tao wrote:
> > This series implements full image preallocation to create a non-sparse image
> > file at creation time, both for raw and qcow2 format. The purpose is to avoid
> > performance deterioration of the guest cause by sparse image.
> > 
> > v5:
> >   - fix wrong calculation of qcow2 metadata size in v4
> >   - remove raw_preallocate2()
> >   - better error out path in raw_create()
> >   - fix coding style
> > 
> > 
> > Hu Tao (4):
> >   qapi: introduce PreallocMode and a new PreallocMode full.
> >   raw, qcow2: don't convert file size to sector size
> >   raw-posix: Add full image preallocation option
> >   qcow2: Add full image preallocation option
> > 
> >  block/qcow2.c     | 91 ++++++++++++++++++++++++++++++++++++++++++++++++-------
> >  block/raw-posix.c | 45 +++++++++++++++++++++------
> >  qapi-schema.json  | 12 ++++++++
> >  3 files changed, 128 insertions(+), 20 deletions(-)
> 
> Besides the comments that have been made, looks good.  The metadata size
> calculation is correct now.

Thanks for review! I'll post v6 to address comments by Fam and Eric.