[Qemu-devel] [PATCH v10 0/6] qemu-img: add preallocation=full

[Qemu-devel] [PATCH v10 0/6] qemu-img: add preallocation=full

[Hu Tao]

The purpose of this series is to use posix_fallocate() when creating
img file to ensure there are disk space for it which is way fast than
acturally writing to disk. But this only works in file system level.
For cases like thin provisioning, an option full preallocation is
added to write zeros to storage to ensure disk space.

note: there are two false positives of checkpatch.pl to patch 01.

changes to v9:

 - use ROUND_UP to do round up
 - split the round up into its own patch and add test case
 - new patch rename parse_enum_option to qapi_enum_parse and make it public
 - reuse qapi_enum_parse

Hu Tao (6):
  block: round up file size to nearest sector
  raw, qcow2: don't convert file size to sector size
  rename parse_enum_option to qapi_enum_parse and make it public
  qapi: introduce PreallocMode and a new PreallocMode full.
  raw-posix: Add full image preallocation option
  qcow2: Add full image preallocation option

 block/qcow2.c              | 102 ++++++++++++++++++++++++++++++++++++++-------
 block/raw-posix.c          |  61 +++++++++++++++++++++++----
 block/raw-win32.c          |   4 +-
 blockdev.c                 |  22 +---------
 include/qapi/util.h        |  17 ++++++++
 qapi-schema.json           |  14 +++++++
 qapi/Makefile.objs         |   2 +-
 qapi/qapi-util.c           |  32 ++++++++++++++
 tests/qemu-iotests/082.out |  54 ++++++++++++------------
 tests/qemu-iotests/093     |  64 ++++++++++++++++++++++++++++
 tests/qemu-iotests/093.out |  14 +++++++
 tests/qemu-iotests/group   |   1 +
 12 files changed, 313 insertions(+), 74 deletions(-)
 create mode 100644 include/qapi/util.h
 create mode 100644 qapi/qapi-util.c
 create mode 100755 tests/qemu-iotests/093
 create mode 100644 tests/qemu-iotests/093.out

-- 
1.9.3

[Qemu-devel] [PATCH v10 1/6] block: round up file size to nearest sector

[Hu Tao]

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2.c              |  2 +-
 block/raw-posix.c          |  2 +-
 block/raw-win32.c          |  2 +-
 tests/qemu-iotests/093     | 64 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/093.out | 14 ++++++++++
 tests/qemu-iotests/group   |  1 +
 6 files changed, 82 insertions(+), 3 deletions(-)
 create mode 100755 tests/qemu-iotests/093
 create mode 100644 tests/qemu-iotests/093.out

diff --git a/block/qcow2.c b/block/qcow2.c
index a54d2ba..75b28cd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1778,7 +1778,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;
+            sectors = DIV_ROUND_UP(options->value.n, BDRV_SECTOR_SIZE);
         } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
             backing_file = options->value.s;
         } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FMT)) {
diff --git a/block/raw-posix.c b/block/raw-posix.c
index c2b30be..c5b81e1 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1285,7 +1285,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 = DIV_ROUND_UP(options->value.n, BDRV_SECTOR_SIZE);
         }
         options++;
     }
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 324e818..8f83fab 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -514,7 +514,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 / 512;
+            total_size = DIV_ROUND_UP(options->value.n, BDRV_SECTOR_SIZE);
         }
         options++;
     }
diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
new file mode 100755
index 0000000..02316d1
--- /dev/null
+++ b/tests/qemu-iotests/093
@@ -0,0 +1,64 @@
+#!/bin/bash
+#
+# Test qcow2 preallocation with different cluster_sizes
+#
+# Copyright (C) 2014 Fujitsu.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=hutao@cn.fujitsu.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+function test_qemu_img()
+{
+    echo qemu-img "$@" | _filter_testdir
+    $QEMU_IMG "$@" 2>&1 | _filter_testdir
+    echo
+}
+
+echo "=== Check qemu-img info output ==="
+echo
+image_sizes="1024 1234"
+
+for s in $image_sizes; do
+    _make_test_img $s
+    _img_info
+done
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/093.out b/tests/qemu-iotests/093.out
new file mode 100644
index 0000000..766b512
--- /dev/null
+++ b/tests/qemu-iotests/093.out
@@ -0,0 +1,14 @@
+QA output created by 093
+=== Check qemu-img info output ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024 
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 1.0K (1024 bytes)
+cluster_size: 65536
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1234 
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 1.5K (1536 bytes)
+cluster_size: 65536
+***done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 0f07440..304b596 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -99,3 +99,4 @@
 090 rw auto quick
 091 rw auto
 092 rw auto quick
+093 rw auto
-- 
1.9.3

Re: [Qemu-devel] [PATCH v10 1/6] block: round up file size to nearest sector

[Max Reitz]

On 12.06.2014 05:54, Hu Tao wrote:
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>   block/qcow2.c              |  2 +-
>   block/raw-posix.c          |  2 +-
>   block/raw-win32.c          |  2 +-
>   tests/qemu-iotests/093     | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/093.out | 14 ++++++++++
>   tests/qemu-iotests/group   |  1 +
>   6 files changed, 82 insertions(+), 3 deletions(-)
>   create mode 100755 tests/qemu-iotests/093
>   create mode 100644 tests/qemu-iotests/093.out

Reviewed-by: Max Reitz <mreitz@redhat.com>

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

[Hu Tao]

and avoid converting it back later. And round up file size to nearest
sector.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index 75b28cd..6732e7c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1714,7 +1714,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;
@@ -1767,7 +1767,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;
     int prealloc = 0;
@@ -1778,7 +1778,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 = DIV_ROUND_UP(options->value.n, BDRV_SECTOR_SIZE);
+            size = ROUND_UP(options->value.n, BDRV_SECTOR_SIZE);
         } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
             backing_file = options->value.s;
         } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FMT)) {
@@ -1829,7 +1829,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 (local_err) {
         error_propagate(errp, local_err);
diff --git a/block/raw-posix.c b/block/raw-posix.c
index c5b81e1..35057f0 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1285,7 +1285,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 = DIV_ROUND_UP(options->value.n, BDRV_SECTOR_SIZE);
+            total_size = ROUND_UP(options->value.n, BDRV_SECTOR_SIZE);
         }
         options++;
     }
@@ -1296,7 +1296,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");
         }
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 8f83fab..f681648 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -514,7 +514,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 = DIV_ROUND_UP(options->value.n, BDRV_SECTOR_SIZE);
+            total_size = ROUND_UP(options->value.n, BDRV_SECTOR_SIZE);
         }
         options++;
     }
@@ -526,7 +526,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
         return -EIO;
     }
     set_sparse(fd);
-    ftruncate(fd, total_size * 512);
+    ftruncate(fd, total_size);
     qemu_close(fd);
     return 0;
 }
-- 
1.9.3

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

[Max Reitz]

On 12.06.2014 05:54, Hu Tao wrote:
> and avoid converting it back later. And round up file size to nearest
> sector.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>   block/qcow2.c     | 8 ++++----
>   block/raw-posix.c | 4 ++--
>   block/raw-win32.c | 4 ++--
>   3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 75b28cd..6732e7c 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1714,7 +1714,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;
> @@ -1767,7 +1767,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;
>       int prealloc = 0;
> @@ -1778,7 +1778,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 = DIV_ROUND_UP(options->value.n, BDRV_SECTOR_SIZE);
> +            size = ROUND_UP(options->value.n, BDRV_SECTOR_SIZE);

I'm not even sure whether the ROUND_UP() is needed anymore, as qcow2 
should only care about clusters. However, we still have that more or 
less arbitrary "sector" unit in the block layer (although I think Markus 
is trying to get rid of it), so it's probably for the better to align 
the size here.

Reviewed-by: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCH v10 3/6] rename parse_enum_option to qapi_enum_parse and make it public

[Hu Tao]

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c          | 22 ++--------------------
 include/qapi/util.h | 17 +++++++++++++++++
 qapi/Makefile.objs  |  2 +-
 qapi/qapi-util.c    | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 21 deletions(-)
 create mode 100644 include/qapi/util.h
 create mode 100644 qapi/qapi-util.c

diff --git a/blockdev.c b/blockdev.c
index 4cbcc56..9adfdbb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -39,6 +39,7 @@
 #include "qapi/qmp/types.h"
 #include "qapi-visit.h"
 #include "qapi/qmp-output-visitor.h"
+#include "qapi/util.h"
 #include "sysemu/sysemu.h"
 #include "block/block_int.h"
 #include "qmp-commands.h"
@@ -287,25 +288,6 @@ static int parse_block_error_action(const char *buf, bool is_read, Error **errp)
     }
 }
 
-static inline int parse_enum_option(const char *lookup[], const char *buf,
-                                    int max, int def, Error **errp)
-{
-    int i;
-
-    if (!buf) {
-        return def;
-    }
-
-    for (i = 0; i < max; i++) {
-        if (!strcmp(buf, lookup[i])) {
-            return i;
-        }
-    }
-
-    error_setg(errp, "invalid parameter value: %s", buf);
-    return def;
-}
-
 static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
 {
     if (throttle_conflicting(cfg)) {
@@ -472,7 +454,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
     }
 
     detect_zeroes =
-        parse_enum_option(BlockdevDetectZeroesOptions_lookup,
+        qapi_enum_parse(BlockdevDetectZeroesOptions_lookup,
                           qemu_opt_get(opts, "detect-zeroes"),
                           BLOCKDEV_DETECT_ZEROES_OPTIONS_MAX,
                           BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
diff --git a/include/qapi/util.h b/include/qapi/util.h
new file mode 100644
index 0000000..de9238b
--- /dev/null
+++ b/include/qapi/util.h
@@ -0,0 +1,17 @@
+/*
+ * QAPI util functions
+ *
+ * Copyright Fujitsu, Inc. 2014
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef QAPI_UTIL_H
+#define QAPI_UTIL_H
+
+int qapi_enum_parse(const char *lookup[], const char *buf,
+                    int max, int def, Error **errp);
+
+#endif
diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
index 1f9c973..97c98c5 100644
--- a/qapi/Makefile.objs
+++ b/qapi/Makefile.objs
@@ -2,4 +2,4 @@ util-obj-y = qapi-visit-core.o qapi-dealloc-visitor.o qmp-input-visitor.o
 util-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o
 util-obj-y += string-input-visitor.o string-output-visitor.o
 
-util-obj-y += opts-visitor.o
+util-obj-y += opts-visitor.o qapi-util.o
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
new file mode 100644
index 0000000..3e4e07c
--- /dev/null
+++ b/qapi/qapi-util.c
@@ -0,0 +1,32 @@
+/*
+ * QAPI util functions
+ *
+ * Copyright Fujitsu, Inc. 2014
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qemu-common.h"
+#include "qapi/error.h"
+#include "qapi/util.h"
+
+int qapi_enum_parse(const char *lookup[], const char *buf,
+                    int max, int def, Error **errp)
+{
+    int i;
+
+    if (!buf) {
+        return def;
+    }
+
+    for (i = 0; i < max; i++) {
+        if (!strcmp(buf, lookup[i])) {
+            return i;
+        }
+    }
+
+    error_setg(errp, "invalid parameter value: %s", buf);
+    return def;
+}
-- 
1.9.3

Re: [Qemu-devel] [PATCH v10 3/6] rename parse_enum_option to qapi_enum_parse and make it public

[Max Reitz]

On 12.06.2014 05:54, Hu Tao wrote:
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> ---
>   blockdev.c          | 22 ++--------------------
>   include/qapi/util.h | 17 +++++++++++++++++
>   qapi/Makefile.objs  |  2 +-
>   qapi/qapi-util.c    | 32 ++++++++++++++++++++++++++++++++
>   4 files changed, 52 insertions(+), 21 deletions(-)
>   create mode 100644 include/qapi/util.h
>   create mode 100644 qapi/qapi-util.c
>
> diff --git a/blockdev.c b/blockdev.c
> index 4cbcc56..9adfdbb 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -39,6 +39,7 @@
>   #include "qapi/qmp/types.h"
>   #include "qapi-visit.h"
>   #include "qapi/qmp-output-visitor.h"
> +#include "qapi/util.h"
>   #include "sysemu/sysemu.h"
>   #include "block/block_int.h"
>   #include "qmp-commands.h"
> @@ -287,25 +288,6 @@ static int parse_block_error_action(const char *buf, bool is_read, Error **errp)
>       }
>   }
>   
> -static inline int parse_enum_option(const char *lookup[], const char *buf,
> -                                    int max, int def, Error **errp)
> -{
> -    int i;
> -
> -    if (!buf) {
> -        return def;
> -    }
> -
> -    for (i = 0; i < max; i++) {
> -        if (!strcmp(buf, lookup[i])) {
> -            return i;
> -        }
> -    }
> -
> -    error_setg(errp, "invalid parameter value: %s", buf);
> -    return def;
> -}
> -
>   static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
>   {
>       if (throttle_conflicting(cfg)) {
> @@ -472,7 +454,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>       }
>   
>       detect_zeroes =
> -        parse_enum_option(BlockdevDetectZeroesOptions_lookup,
> +        qapi_enum_parse(BlockdevDetectZeroesOptions_lookup,
>                             qemu_opt_get(opts, "detect-zeroes"),
>                             BLOCKDEV_DETECT_ZEROES_OPTIONS_MAX,
>                             BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,

Please adapt the indentation of the other parameters.

Other than that:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v10 3/6] rename parse_enum_option to qapi_enum_parse and make it public

[Hu Tao]

On Sat, Jun 14, 2014 at 09:07:19PM +0200, Max Reitz wrote:
> On 12.06.2014 05:54, Hu Tao wrote:
> >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >Suggested-by: Markus Armbruster <armbru@redhat.com>
> >---
> >  blockdev.c          | 22 ++--------------------
> >  include/qapi/util.h | 17 +++++++++++++++++
> >  qapi/Makefile.objs  |  2 +-
> >  qapi/qapi-util.c    | 32 ++++++++++++++++++++++++++++++++
> >  4 files changed, 52 insertions(+), 21 deletions(-)
> >  create mode 100644 include/qapi/util.h
> >  create mode 100644 qapi/qapi-util.c
> >
> >diff --git a/blockdev.c b/blockdev.c
> >index 4cbcc56..9adfdbb 100644
> >--- a/blockdev.c
> >+++ b/blockdev.c
> >@@ -39,6 +39,7 @@
> >  #include "qapi/qmp/types.h"
> >  #include "qapi-visit.h"
> >  #include "qapi/qmp-output-visitor.h"
> >+#include "qapi/util.h"
> >  #include "sysemu/sysemu.h"
> >  #include "block/block_int.h"
> >  #include "qmp-commands.h"
> >@@ -287,25 +288,6 @@ static int parse_block_error_action(const char *buf, bool is_read, Error **errp)
> >      }
> >  }
> >-static inline int parse_enum_option(const char *lookup[], const char *buf,
> >-                                    int max, int def, Error **errp)
> >-{
> >-    int i;
> >-
> >-    if (!buf) {
> >-        return def;
> >-    }
> >-
> >-    for (i = 0; i < max; i++) {
> >-        if (!strcmp(buf, lookup[i])) {
> >-            return i;
> >-        }
> >-    }
> >-
> >-    error_setg(errp, "invalid parameter value: %s", buf);
> >-    return def;
> >-}
> >-
> >  static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
> >  {
> >      if (throttle_conflicting(cfg)) {
> >@@ -472,7 +454,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
> >      }
> >      detect_zeroes =
> >-        parse_enum_option(BlockdevDetectZeroesOptions_lookup,
> >+        qapi_enum_parse(BlockdevDetectZeroesOptions_lookup,
> >                            qemu_opt_get(opts, "detect-zeroes"),
> >                            BLOCKDEV_DETECT_ZEROES_OPTIONS_MAX,
> >                            BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
> 
> Please adapt the indentation of the other parameters.

Sure.

> 
> Other than that:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCH v10 4/6] qapi: introduce PreallocMode and a new PreallocMode full.

[Hu Tao]

This patch prepares for the subsequent patches.

Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2.c    |  8 ++++----
 qapi-schema.json | 14 ++++++++++++++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6732e7c..2e0b83c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1593,7 +1593,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)
 {
@@ -1770,7 +1770,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
     uint64_t size = 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;
@@ -1791,9 +1791,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 14b498b..f5ccb31 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3077,3 +3077,17 @@
               'btn'     : 'InputBtnEvent',
               'rel'     : 'InputMoveEvent',
               'abs'     : 'InputMoveEvent' } }
+
+##
+# @PreallocMode
+#
+# Preallocation mode of QEMU image file
+#
+# @off: no preallocation
+# @metadata: preallocate only for metadata
+# @full: preallocate all data, including metadata
+#
+# Since 2.1
+##
+{ 'enum': 'PreallocMode',
+  'data': [ 'off', 'metadata', 'full' ] }
-- 
1.9.3

Re: [Qemu-devel] [PATCH v10 4/6] qapi: introduce PreallocMode and a new PreallocMode full.

[Max Reitz]

On 12.06.2014 05:54, Hu Tao wrote:
> This patch prepares for the subsequent patches.
>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>   block/qcow2.c    |  8 ++++----
>   qapi-schema.json | 14 ++++++++++++++
>   2 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6732e7c..2e0b83c 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1593,7 +1593,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)
>   {
> @@ -1770,7 +1770,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
>       uint64_t size = 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;
> @@ -1791,9 +1791,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 14b498b..f5ccb31 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json

As of commit 5db15096 (Benoît's "Extract QAPI block commands" series), 
block-related QAPI definitions should go into qapi/block.json or 
qapi/block-core.json. I think the following definition should now go 
into qapi/block-core.json instead of qapi-schema.json.

Apart from that, this patch looks good.

Max

> @@ -3077,3 +3077,17 @@
>                 'btn'     : 'InputBtnEvent',
>                 'rel'     : 'InputMoveEvent',
>                 'abs'     : 'InputMoveEvent' } }
> +
> +##
> +# @PreallocMode
> +#
> +# Preallocation mode of QEMU image file
> +#
> +# @off: no preallocation
> +# @metadata: preallocate only for metadata
> +# @full: preallocate all data, including metadata
> +#
> +# Since 2.1
> +##
> +{ 'enum': 'PreallocMode',
> +  'data': [ 'off', 'metadata', 'full' ] }

Re: [Qemu-devel] [PATCH v10 4/6] qapi: introduce PreallocMode and a new PreallocMode full.

[Hu Tao]

On Sat, Jun 14, 2014 at 09:17:02PM +0200, Max Reitz wrote:
> On 12.06.2014 05:54, Hu Tao wrote:
> >This patch prepares for the subsequent patches.
> >
> >Reviewed-by: Fam Zheng <famz@redhat.com>
> >Reviewed-by: Eric Blake <eblake@redhat.com>
> >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >---
> >  block/qcow2.c    |  8 ++++----
> >  qapi-schema.json | 14 ++++++++++++++
> >  2 files changed, 18 insertions(+), 4 deletions(-)
> >
> >diff --git a/block/qcow2.c b/block/qcow2.c
> >index 6732e7c..2e0b83c 100644
> >--- a/block/qcow2.c
> >+++ b/block/qcow2.c
> >@@ -1593,7 +1593,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)
> >  {
> >@@ -1770,7 +1770,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
> >      uint64_t size = 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;
> >@@ -1791,9 +1791,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 14b498b..f5ccb31 100644
> >--- a/qapi-schema.json
> >+++ b/qapi-schema.json
> 
> As of commit 5db15096 (Benoît's "Extract QAPI block commands"
> series), block-related QAPI definitions should go into
> qapi/block.json or qapi/block-core.json. I think the following
> definition should now go into qapi/block-core.json instead of
> qapi-schema.json.
> 
> Apart from that, this patch looks good.
> 
> Max

Thanks, I'll fix in next version.

Hu

[Qemu-devel] [PATCH v10 5/6] 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 | 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;
+            }
         }
         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;
         }
+        fsync(fd);
+        g_free(buf);
     }
+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;
 }
 
@@ -1479,6 +1519,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, metadata, full)"
+    },
     { NULL }
 };
 
-- 
1.9.3

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

[Max Reitz]

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 <hutao@cn.fujitsu.com>
> ---
>   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.

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.

Max

> +        fsync(fd);
> +        g_free(buf);
>       }
> +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;
>   }
>   
> @@ -1479,6 +1519,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, metadata, full)"
> +    },
>       { NULL }
>   };
>   

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 <hutao@cn.fujitsu.com>
> >---
> >  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

[Qemu-devel] [PATCH v10 6/6] 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              | 90 ++++++++++++++++++++++++++++++++++++++++------
 tests/qemu-iotests/082.out | 54 ++++++++++++++--------------
 2 files changed, 107 insertions(+), 37 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2e0b83c..8c87c1a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -30,6 +30,7 @@
 #include "qemu/error-report.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qbool.h"
+#include "qapi/util.h"
 #include "trace.h"
 
 /*
@@ -1597,6 +1598,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;
@@ -1626,10 +1628,78 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     Error *local_err = NULL;
     int ret;
 
+    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_METADATA) {
+        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);
+        if (prealloc == PREALLOC_MODE_FULL) {
+            set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC, "full");
+        } else if (prealloc == PREALLOC_MODE_METADATA) {
+            set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC, "metadata");
+        }
+
+        options = alloc_options;
+    }
+
     ret = bdrv_create_file(filename, options, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
-        return ret;
+        goto out_options;
     }
 
     bs = NULL;
@@ -1637,7 +1707,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
                     NULL, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
-        return ret;
+        goto out_options;
     }
 
     /* Write the header */
@@ -1759,6 +1829,8 @@ out:
     if (bs) {
         bdrv_unref(bs);
     }
+out_options:
+    free_option_parameters(alloc_options);
     return ret;
 }
 
@@ -1790,13 +1862,11 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
                 cluster_size = options->value.n;
             }
         } 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, "metadata")) {
-                prealloc = PREALLOC_MODE_METADATA;
-            } else {
-                error_setg(errp, "Invalid preallocation mode: '%s'",
-                           options->value.s);
+            prealloc = qapi_enum_parse(PreallocMode_lookup, options->value.s,
+                                       PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
+                                       &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
                 return -EINVAL;
             }
         } else if (!strcmp(options->name, BLOCK_OPT_COMPAT_LEVEL)) {
@@ -2359,7 +2429,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,
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index 28309a0..5689802 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -64,7 +64,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: create -f qcow2 -o ? TEST_DIR/t.qcow2 128M
@@ -75,7 +75,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: create -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 128M
@@ -86,7 +86,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: create -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 128M
@@ -97,7 +97,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: create -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 128M
@@ -108,7 +108,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: create -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 128M
@@ -119,7 +119,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: create -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 128M
@@ -130,7 +130,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: create -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 128M
@@ -141,7 +141,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 128M
@@ -167,7 +167,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: create -o help
@@ -245,7 +245,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: convert -O qcow2 -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -256,7 +256,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: convert -O qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -267,7 +267,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: convert -O qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -278,7 +278,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: convert -O qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -289,7 +289,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: convert -O qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -300,7 +300,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: convert -O qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -311,7 +311,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: convert -O qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -322,7 +322,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
@@ -348,7 +348,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: convert -o help
@@ -415,7 +415,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: amend -f qcow2 -o ? TEST_DIR/t.qcow2
@@ -426,7 +426,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: amend -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2
@@ -437,7 +437,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: amend -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2
@@ -448,7 +448,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: amend -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2
@@ -459,7 +459,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: amend -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2
@@ -470,7 +470,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: amend -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2
@@ -481,7 +481,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: amend -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2
@@ -492,7 +492,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2
@@ -520,7 +520,7 @@ backing_file     File name of a base image
 backing_fmt      Image format of the base image
 encryption       Encrypt the image
 cluster_size     qcow2 cluster size
-preallocation    Preallocation mode (allowed values: off, metadata)
+preallocation    Preallocation mode (allowed values: off, metadata, full)
 lazy_refcounts   Postpone refcount updates
 
 Testing: convert -o help
-- 
1.9.3

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

[Max Reitz]

On 12.06.2014 05:54, Hu Tao wrote:
> 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              | 90 ++++++++++++++++++++++++++++++++++++++++------
>   tests/qemu-iotests/082.out | 54 ++++++++++++++--------------
>   2 files changed, 107 insertions(+), 37 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2e0b83c..8c87c1a 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -30,6 +30,7 @@
>   #include "qemu/error-report.h"
>   #include "qapi/qmp/qerror.h"
>   #include "qapi/qmp/qbool.h"
> +#include "qapi/util.h"
>   #include "trace.h"
>   
>   /*
> @@ -1597,6 +1598,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;
> @@ -1626,10 +1628,78 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>       Error *local_err = NULL;
>       int ret;
>   
> +    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_METADATA) {
> +        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);

Hm, you could use the function minimal_blob_size() from my "qemu-img: 
Implement commit like QMP" series. It calculates the minimal size 
required for an L1 table, the refcount table and all refcount blocks for 
an image with a given number of guest sectors and a given number of 
"overhead", which are clusters that should be covered by the refcount 
structures in addition to L1+Reftable+Refblocks. To use it here, you 
would have to use it probably like this:

meta_size = (1 + minimal_blob_size(total_size / BDRV_SECTOR_SIZE, 
cluster_bits, cluster_bits - BDRV_SECTOR_BITS, 1 + l2_clusters + 
(total_size >> cluster_bits)) + l2_clusters) << cluster_bits;

The first parameter is the number of guest sectors; the second is 
cluster_bits; the third is parameter x, so that (1 << x) == number of 
sectors per cluster; and the fourth is the number of clusters which 
should be covered by the refcount structures in addition to 
L1+Reftable+Refblocks. These are the image header (1 cluster), the L2 
tables (l2_clusters, which you'd have to calculate yourself (as you are 
already doing in this patch)) and the clusters for guest data 
(total_size >> cluster_bits).

The function then returns the number of clusters required from 
L1+Reftable+Refblocks to which the image header cluster and the L2 
clusters have to be added to get the full metadata size.

I'm suggesting you use this function (even though it's not merged yet, 
but fully reviewed), because I guess it's always likely to have some 
kind of small mistake in the size calculation for the refcount 
structures; and therefore it's better to have a single place where these 
mistakes can occur.


As for your own calculation code: You should round all structure sizes 
separately to full clusters (the L1 table, the L2 tables, the refcount 
table and the refcount blocks). Especially this makes the refcount 
structure size so "hard" to calculate (or at least error-prone), as this 
has to be considered for the recursive calculation you demonstrated in 
your comment.

> +
> +        set_option_parameter_int(alloc_options, BLOCK_OPT_SIZE,
> +                                 total_size + meta_size);

Wouldn't it suffice for preallocation=metadata to only allocate meta_size?

If we allocate total_size + meta_size for metadata preallocation as 
well, both metadata and full preallocation are the same (as far as I can 
see). But if we only allocate meta_size, we have to rework 
preallocate(): This function really "allocates" the data clusters from 
qcow2's point of view, which means they are holes in the raw file 
whereas all metadata should ideally be contained in the blob sized 
meta_size at the start of the file and the rest should be completely unused.

With the current preallocate(), we either have to allocate everything 
(including data), or can't guarantee (and it will in fact never happen) 
that all metadata clusters fit into the meta_size sized blob, because it 
creates a mixture of interleaved data and metadata. The only way I can 
think of to really fix this is to rework preallocate() so that it 
doesn't use the regular qcow2_alloc_cluster_offset() function anymore 
but fills all metadata tables by itself (which should not be too hard 
for a simple linear preallocation of everything) and places all data 
clusters behind the metadata blob.

> +        if (prealloc == PREALLOC_MODE_FULL) {
> +            set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC, "full");
> +        } else if (prealloc == PREALLOC_MODE_METADATA) {
> +            set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC, "metadata");
> +        }

How about set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC, 
PreallocMode_lookup[prealloc]); ?

Max

> +
> +        options = alloc_options;
> +    }
> +
>       ret = bdrv_create_file(filename, options, &local_err);
>       if (ret < 0) {
>           error_propagate(errp, local_err);
> -        return ret;
> +        goto out_options;
>       }
>   
>       bs = NULL;
> @@ -1637,7 +1707,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>                       NULL, &local_err);
>       if (ret < 0) {
>           error_propagate(errp, local_err);
> -        return ret;
> +        goto out_options;
>       }
>   
>       /* Write the header */
> @@ -1759,6 +1829,8 @@ out:
>       if (bs) {
>           bdrv_unref(bs);
>       }
> +out_options:
> +    free_option_parameters(alloc_options);
>       return ret;
>   }
>   
> @@ -1790,13 +1862,11 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
>                   cluster_size = options->value.n;
>               }
>           } 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, "metadata")) {
> -                prealloc = PREALLOC_MODE_METADATA;
> -            } else {
> -                error_setg(errp, "Invalid preallocation mode: '%s'",
> -                           options->value.s);
> +            prealloc = qapi_enum_parse(PreallocMode_lookup, options->value.s,
> +                                       PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
> +                                       &local_err);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
>                   return -EINVAL;
>               }
>           } else if (!strcmp(options->name, BLOCK_OPT_COMPAT_LEVEL)) {
> @@ -2359,7 +2429,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,
> diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
> index 28309a0..5689802 100644
> --- a/tests/qemu-iotests/082.out
> +++ b/tests/qemu-iotests/082.out
> @@ -64,7 +64,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: create -f qcow2 -o ? TEST_DIR/t.qcow2 128M
> @@ -75,7 +75,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: create -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 128M
> @@ -86,7 +86,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: create -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 128M
> @@ -97,7 +97,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: create -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 128M
> @@ -108,7 +108,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: create -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 128M
> @@ -119,7 +119,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: create -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 128M
> @@ -130,7 +130,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: create -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 128M
> @@ -141,7 +141,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 128M
> @@ -167,7 +167,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: create -o help
> @@ -245,7 +245,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: convert -O qcow2 -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> @@ -256,7 +256,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: convert -O qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> @@ -267,7 +267,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: convert -O qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> @@ -278,7 +278,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: convert -O qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> @@ -289,7 +289,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: convert -O qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> @@ -300,7 +300,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: convert -O qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> @@ -311,7 +311,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: convert -O qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> @@ -322,7 +322,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> @@ -348,7 +348,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: convert -o help
> @@ -415,7 +415,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: amend -f qcow2 -o ? TEST_DIR/t.qcow2
> @@ -426,7 +426,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: amend -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2
> @@ -437,7 +437,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: amend -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2
> @@ -448,7 +448,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: amend -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2
> @@ -459,7 +459,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: amend -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2
> @@ -470,7 +470,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: amend -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2
> @@ -481,7 +481,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: amend -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2
> @@ -492,7 +492,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2
> @@ -520,7 +520,7 @@ backing_file     File name of a base image
>   backing_fmt      Image format of the base image
>   encryption       Encrypt the image
>   cluster_size     qcow2 cluster size
> -preallocation    Preallocation mode (allowed values: off, metadata)
> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>   lazy_refcounts   Postpone refcount updates
>   
>   Testing: convert -o help

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

[Hu Tao]

On Sat, Jun 14, 2014 at 10:37:33PM +0200, Max Reitz wrote:
> On 12.06.2014 05:54, Hu Tao wrote:
> >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              | 90 ++++++++++++++++++++++++++++++++++++++++------
> >  tests/qemu-iotests/082.out | 54 ++++++++++++++--------------
> >  2 files changed, 107 insertions(+), 37 deletions(-)
> >
> >diff --git a/block/qcow2.c b/block/qcow2.c
> >index 2e0b83c..8c87c1a 100644
> >--- a/block/qcow2.c
> >+++ b/block/qcow2.c
> >@@ -30,6 +30,7 @@
> >  #include "qemu/error-report.h"
> >  #include "qapi/qmp/qerror.h"
> >  #include "qapi/qmp/qbool.h"
> >+#include "qapi/util.h"
> >  #include "trace.h"
> >  /*
> >@@ -1597,6 +1598,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;
> >@@ -1626,10 +1628,78 @@ static int qcow2_create2(const char *filename, int64_t total_size,
> >      Error *local_err = NULL;
> >      int ret;
> >+    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_METADATA) {
> >+        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);
> 
> Hm, you could use the function minimal_blob_size() from my
> "qemu-img: Implement commit like QMP" series. It calculates the
> minimal size required for an L1 table, the refcount table and all
> refcount blocks for an image with a given number of guest sectors
> and a given number of "overhead", which are clusters that should be
> covered by the refcount structures in addition to
> L1+Reftable+Refblocks. To use it here, you would have to use it
> probably like this:
> 
> meta_size = (1 + minimal_blob_size(total_size / BDRV_SECTOR_SIZE,
> cluster_bits, cluster_bits - BDRV_SECTOR_BITS, 1 + l2_clusters +
> (total_size >> cluster_bits)) + l2_clusters) << cluster_bits;
> 
> The first parameter is the number of guest sectors; the second is
> cluster_bits; the third is parameter x, so that (1 << x) == number
> of sectors per cluster; and the fourth is the number of clusters
> which should be covered by the refcount structures in addition to
> L1+Reftable+Refblocks. These are the image header (1 cluster), the
> L2 tables (l2_clusters, which you'd have to calculate yourself (as
> you are already doing in this patch)) and the clusters for guest
> data (total_size >> cluster_bits).
> 
> The function then returns the number of clusters required from
> L1+Reftable+Refblocks to which the image header cluster and the L2
> clusters have to be added to get the full metadata size.
> 
> I'm suggesting you use this function (even though it's not merged
> yet, but fully reviewed), because I guess it's always likely to have
> some kind of small mistake in the size calculation for the refcount
> structures; and therefore it's better to have a single place where
> these mistakes can occur.

Thanks! I used minimal_blob_size() from your patch. But tests show that
the calculated meta size (and the image file size) is larger than the
number calculated from my patch and without patch(file size).

test command:
qemu-img create -f qcow2 -o preallocation=metadata delme.img 8588490000

result:

without patch:

Formatting 'delme.img', fmt=qcow2 size=8588490000 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off 
-rw-r--r-- 1 root root 8590065664 Jun 20 15:02 delme.img


my patch:

Formatting 'delme.img', fmt=qcow2 size=8588490000 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off 
meta_size: 1507328, 1507328
total_size: 8588492800, 8588492800
-rw-r--r-- 1 root root 8590065664 Jun 20 15:01 delme.img


using minimal_blob_size:

Formatting 'delme.img', fmt=qcow2 size=8588490000 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off 
-rw-r--r-- 1 root root 8590131200 Jun 20 15:03 delme.img


I need to review minimal_blob_size() further.

> 
> 
> As for your own calculation code: You should round all structure
> sizes separately to full clusters (the L1 table, the L2 tables, the
> refcount table and the refcount blocks). Especially this makes the
> refcount structure size so "hard" to calculate (or at least
> error-prone), as this has to be considered for the recursive
> calculation you demonstrated in your comment.
> 
> >+
> >+        set_option_parameter_int(alloc_options, BLOCK_OPT_SIZE,
> >+                                 total_size + meta_size);
> 
> Wouldn't it suffice for preallocation=metadata to only allocate meta_size?
> 
> If we allocate total_size + meta_size for metadata preallocation as
> well, both metadata and full preallocation are the same (as far as I
> can see). But if we only allocate meta_size, we have to rework
> preallocate(): This function really "allocates" the data clusters
> from qcow2's point of view, which means they are holes in the raw
> file whereas all metadata should ideally be contained in the blob
> sized meta_size at the start of the file and the rest should be
> completely unused.
> 
> With the current preallocate(), we either have to allocate
> everything (including data), or can't guarantee (and it will in fact
> never happen) that all metadata clusters fit into the meta_size
> sized blob, because it creates a mixture of interleaved data and
> metadata. The only way I can think of to really fix this is to
> rework preallocate() so that it doesn't use the regular
> qcow2_alloc_cluster_offset() function anymore but fills all metadata
> tables by itself (which should not be too hard for a simple linear
> preallocation of everything) and places all data clusters behind the
> metadata blob.
> 
> >+        if (prealloc == PREALLOC_MODE_FULL) {
> >+            set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC, "full");
> >+        } else if (prealloc == PREALLOC_MODE_METADATA) {
> >+            set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC, "metadata");
> >+        }
> 
> How about set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC,
> PreallocMode_lookup[prealloc]); ?
> 
> Max
> 
> >+
> >+        options = alloc_options;
> >+    }
> >+
> >      ret = bdrv_create_file(filename, options, &local_err);
> >      if (ret < 0) {
> >          error_propagate(errp, local_err);
> >-        return ret;
> >+        goto out_options;
> >      }
> >      bs = NULL;
> >@@ -1637,7 +1707,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
> >                      NULL, &local_err);
> >      if (ret < 0) {
> >          error_propagate(errp, local_err);
> >-        return ret;
> >+        goto out_options;
> >      }
> >      /* Write the header */
> >@@ -1759,6 +1829,8 @@ out:
> >      if (bs) {
> >          bdrv_unref(bs);
> >      }
> >+out_options:
> >+    free_option_parameters(alloc_options);
> >      return ret;
> >  }
> >@@ -1790,13 +1862,11 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
> >                  cluster_size = options->value.n;
> >              }
> >          } 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, "metadata")) {
> >-                prealloc = PREALLOC_MODE_METADATA;
> >-            } else {
> >-                error_setg(errp, "Invalid preallocation mode: '%s'",
> >-                           options->value.s);
> >+            prealloc = qapi_enum_parse(PreallocMode_lookup, options->value.s,
> >+                                       PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
> >+                                       &local_err);
> >+            if (local_err) {
> >+                error_propagate(errp, local_err);
> >                  return -EINVAL;
> >              }
> >          } else if (!strcmp(options->name, BLOCK_OPT_COMPAT_LEVEL)) {
> >@@ -2359,7 +2429,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,
> >diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
> >index 28309a0..5689802 100644
> >--- a/tests/qemu-iotests/082.out
> >+++ b/tests/qemu-iotests/082.out
> >@@ -64,7 +64,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: create -f qcow2 -o ? TEST_DIR/t.qcow2 128M
> >@@ -75,7 +75,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: create -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 128M
> >@@ -86,7 +86,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: create -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 128M
> >@@ -97,7 +97,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: create -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 128M
> >@@ -108,7 +108,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: create -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 128M
> >@@ -119,7 +119,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: create -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 128M
> >@@ -130,7 +130,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: create -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 128M
> >@@ -141,7 +141,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 128M
> >@@ -167,7 +167,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: create -o help
> >@@ -245,7 +245,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: convert -O qcow2 -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >@@ -256,7 +256,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: convert -O qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >@@ -267,7 +267,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: convert -O qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >@@ -278,7 +278,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: convert -O qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >@@ -289,7 +289,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: convert -O qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >@@ -300,7 +300,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: convert -O qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >@@ -311,7 +311,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: convert -O qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >@@ -322,7 +322,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >@@ -348,7 +348,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: convert -o help
> >@@ -415,7 +415,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: amend -f qcow2 -o ? TEST_DIR/t.qcow2
> >@@ -426,7 +426,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: amend -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2
> >@@ -437,7 +437,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: amend -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2
> >@@ -448,7 +448,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: amend -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2
> >@@ -459,7 +459,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: amend -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2
> >@@ -470,7 +470,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: amend -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2
> >@@ -481,7 +481,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: amend -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2
> >@@ -492,7 +492,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2
> >@@ -520,7 +520,7 @@ backing_file     File name of a base image
> >  backing_fmt      Image format of the base image
> >  encryption       Encrypt the image
> >  cluster_size     qcow2 cluster size
> >-preallocation    Preallocation mode (allowed values: off, metadata)
> >+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >  lazy_refcounts   Postpone refcount updates
> >  Testing: convert -o help

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

[Max Reitz]

On 20.06.2014 10:25, Hu Tao wrote:
> On Sat, Jun 14, 2014 at 10:37:33PM +0200, Max Reitz wrote:
>> On 12.06.2014 05:54, Hu Tao wrote:
>>> 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              | 90 ++++++++++++++++++++++++++++++++++++++++------
>>>   tests/qemu-iotests/082.out | 54 ++++++++++++++--------------
>>>   2 files changed, 107 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 2e0b83c..8c87c1a 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -30,6 +30,7 @@
>>>   #include "qemu/error-report.h"
>>>   #include "qapi/qmp/qerror.h"
>>>   #include "qapi/qmp/qbool.h"
>>> +#include "qapi/util.h"
>>>   #include "trace.h"
>>>   /*
>>> @@ -1597,6 +1598,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;
>>> @@ -1626,10 +1628,78 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>>>       Error *local_err = NULL;
>>>       int ret;
>>> +    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_METADATA) {
>>> +        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);
>> Hm, you could use the function minimal_blob_size() from my
>> "qemu-img: Implement commit like QMP" series. It calculates the
>> minimal size required for an L1 table, the refcount table and all
>> refcount blocks for an image with a given number of guest sectors
>> and a given number of "overhead", which are clusters that should be
>> covered by the refcount structures in addition to
>> L1+Reftable+Refblocks. To use it here, you would have to use it
>> probably like this:
>>
>> meta_size = (1 + minimal_blob_size(total_size / BDRV_SECTOR_SIZE,
>> cluster_bits, cluster_bits - BDRV_SECTOR_BITS, 1 + l2_clusters +
>> (total_size >> cluster_bits)) + l2_clusters) << cluster_bits;
>>
>> The first parameter is the number of guest sectors; the second is
>> cluster_bits; the third is parameter x, so that (1 << x) == number
>> of sectors per cluster; and the fourth is the number of clusters
>> which should be covered by the refcount structures in addition to
>> L1+Reftable+Refblocks. These are the image header (1 cluster), the
>> L2 tables (l2_clusters, which you'd have to calculate yourself (as
>> you are already doing in this patch)) and the clusters for guest
>> data (total_size >> cluster_bits).
>>
>> The function then returns the number of clusters required from
>> L1+Reftable+Refblocks to which the image header cluster and the L2
>> clusters have to be added to get the full metadata size.
>>
>> I'm suggesting you use this function (even though it's not merged
>> yet, but fully reviewed), because I guess it's always likely to have
>> some kind of small mistake in the size calculation for the refcount
>> structures; and therefore it's better to have a single place where
>> these mistakes can occur.
> Thanks! I used minimal_blob_size() from your patch. But tests show that
> the calculated meta size (and the image file size) is larger than the
> number calculated from my patch and without patch(file size).
>
> test command:
> qemu-img create -f qcow2 -o preallocation=metadata delme.img 8588490000
>
> result:
>
> without patch:
>
> Formatting 'delme.img', fmt=qcow2 size=8588490000 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off
> -rw-r--r-- 1 root root 8590065664 Jun 20 15:02 delme.img
>
>
> my patch:
>
> Formatting 'delme.img', fmt=qcow2 size=8588490000 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off
> meta_size: 1507328, 1507328
> total_size: 8588492800, 8588492800
> -rw-r--r-- 1 root root 8590065664 Jun 20 15:01 delme.img
>
>
> using minimal_blob_size:
>
> Formatting 'delme.img', fmt=qcow2 size=8588490000 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off
> -rw-r--r-- 1 root root 8590131200 Jun 20 15:03 delme.img

Well, then I'll need to fix minimal_blob_size() anyway. ;-)

But, well, it's just one cluster too much, so I guess it's still fine (I 
mostly made sure to allocate enough space in any case without using 
(potentially imprecise) floating point operations; so it may indeed 
sometimes require too much space, but as long as it's rather 
insignificant, I guess we can live with it).

> I need to review minimal_blob_size() further.

In http://lists.nongnu.org/archive/html/qemu-devel/2014-04/msg04820.html 
I attached a document which explains how I ended up with my formula, 
maybe it'll prove helpful to you.

Max

>
>>
>> As for your own calculation code: You should round all structure
>> sizes separately to full clusters (the L1 table, the L2 tables, the
>> refcount table and the refcount blocks). Especially this makes the
>> refcount structure size so "hard" to calculate (or at least
>> error-prone), as this has to be considered for the recursive
>> calculation you demonstrated in your comment.
>>
>>> +
>>> +        set_option_parameter_int(alloc_options, BLOCK_OPT_SIZE,
>>> +                                 total_size + meta_size);
>> Wouldn't it suffice for preallocation=metadata to only allocate meta_size?
>>
>> If we allocate total_size + meta_size for metadata preallocation as
>> well, both metadata and full preallocation are the same (as far as I
>> can see). But if we only allocate meta_size, we have to rework
>> preallocate(): This function really "allocates" the data clusters
>> from qcow2's point of view, which means they are holes in the raw
>> file whereas all metadata should ideally be contained in the blob
>> sized meta_size at the start of the file and the rest should be
>> completely unused.
>>
>> With the current preallocate(), we either have to allocate
>> everything (including data), or can't guarantee (and it will in fact
>> never happen) that all metadata clusters fit into the meta_size
>> sized blob, because it creates a mixture of interleaved data and
>> metadata. The only way I can think of to really fix this is to
>> rework preallocate() so that it doesn't use the regular
>> qcow2_alloc_cluster_offset() function anymore but fills all metadata
>> tables by itself (which should not be too hard for a simple linear
>> preallocation of everything) and places all data clusters behind the
>> metadata blob.
>>
>>> +        if (prealloc == PREALLOC_MODE_FULL) {
>>> +            set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC, "full");
>>> +        } else if (prealloc == PREALLOC_MODE_METADATA) {
>>> +            set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC, "metadata");
>>> +        }
>> How about set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC,
>> PreallocMode_lookup[prealloc]); ?
>>
>> Max
>>
>>> +
>>> +        options = alloc_options;
>>> +    }
>>> +
>>>       ret = bdrv_create_file(filename, options, &local_err);
>>>       if (ret < 0) {
>>>           error_propagate(errp, local_err);
>>> -        return ret;
>>> +        goto out_options;
>>>       }
>>>       bs = NULL;
>>> @@ -1637,7 +1707,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>>>                       NULL, &local_err);
>>>       if (ret < 0) {
>>>           error_propagate(errp, local_err);
>>> -        return ret;
>>> +        goto out_options;
>>>       }
>>>       /* Write the header */
>>> @@ -1759,6 +1829,8 @@ out:
>>>       if (bs) {
>>>           bdrv_unref(bs);
>>>       }
>>> +out_options:
>>> +    free_option_parameters(alloc_options);
>>>       return ret;
>>>   }
>>> @@ -1790,13 +1862,11 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
>>>                   cluster_size = options->value.n;
>>>               }
>>>           } 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, "metadata")) {
>>> -                prealloc = PREALLOC_MODE_METADATA;
>>> -            } else {
>>> -                error_setg(errp, "Invalid preallocation mode: '%s'",
>>> -                           options->value.s);
>>> +            prealloc = qapi_enum_parse(PreallocMode_lookup, options->value.s,
>>> +                                       PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
>>> +                                       &local_err);
>>> +            if (local_err) {
>>> +                error_propagate(errp, local_err);
>>>                   return -EINVAL;
>>>               }
>>>           } else if (!strcmp(options->name, BLOCK_OPT_COMPAT_LEVEL)) {
>>> @@ -2359,7 +2429,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,
>>> diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
>>> index 28309a0..5689802 100644
>>> --- a/tests/qemu-iotests/082.out
>>> +++ b/tests/qemu-iotests/082.out
>>> @@ -64,7 +64,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: create -f qcow2 -o ? TEST_DIR/t.qcow2 128M
>>> @@ -75,7 +75,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: create -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 128M
>>> @@ -86,7 +86,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: create -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 128M
>>> @@ -97,7 +97,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: create -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 128M
>>> @@ -108,7 +108,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: create -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 128M
>>> @@ -119,7 +119,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: create -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 128M
>>> @@ -130,7 +130,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: create -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 128M
>>> @@ -141,7 +141,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 128M
>>> @@ -167,7 +167,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: create -o help
>>> @@ -245,7 +245,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: convert -O qcow2 -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
>>> @@ -256,7 +256,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: convert -O qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
>>> @@ -267,7 +267,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: convert -O qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
>>> @@ -278,7 +278,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: convert -O qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
>>> @@ -289,7 +289,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: convert -O qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
>>> @@ -300,7 +300,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: convert -O qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
>>> @@ -311,7 +311,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: convert -O qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
>>> @@ -322,7 +322,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
>>> @@ -348,7 +348,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: convert -o help
>>> @@ -415,7 +415,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: amend -f qcow2 -o ? TEST_DIR/t.qcow2
>>> @@ -426,7 +426,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: amend -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2
>>> @@ -437,7 +437,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: amend -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2
>>> @@ -448,7 +448,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: amend -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2
>>> @@ -459,7 +459,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: amend -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2
>>> @@ -470,7 +470,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: amend -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2
>>> @@ -481,7 +481,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: amend -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2
>>> @@ -492,7 +492,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2
>>> @@ -520,7 +520,7 @@ backing_file     File name of a base image
>>>   backing_fmt      Image format of the base image
>>>   encryption       Encrypt the image
>>>   cluster_size     qcow2 cluster size
>>> -preallocation    Preallocation mode (allowed values: off, metadata)
>>> +preallocation    Preallocation mode (allowed values: off, metadata, full)
>>>   lazy_refcounts   Postpone refcount updates
>>>   Testing: convert -o help

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

[Hu Tao]

On Fri, Jun 20, 2014 at 08:37:51PM +0200, Max Reitz wrote:
> On 20.06.2014 10:25, Hu Tao wrote:
> >On Sat, Jun 14, 2014 at 10:37:33PM +0200, Max Reitz wrote:
> >>On 12.06.2014 05:54, Hu Tao wrote:
> >>>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              | 90 ++++++++++++++++++++++++++++++++++++++++------
> >>>  tests/qemu-iotests/082.out | 54 ++++++++++++++--------------
> >>>  2 files changed, 107 insertions(+), 37 deletions(-)
> >>>
> >>>diff --git a/block/qcow2.c b/block/qcow2.c
> >>>index 2e0b83c..8c87c1a 100644
> >>>--- a/block/qcow2.c
> >>>+++ b/block/qcow2.c
> >>>@@ -30,6 +30,7 @@
> >>>  #include "qemu/error-report.h"
> >>>  #include "qapi/qmp/qerror.h"
> >>>  #include "qapi/qmp/qbool.h"
> >>>+#include "qapi/util.h"
> >>>  #include "trace.h"
> >>>  /*
> >>>@@ -1597,6 +1598,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;
> >>>@@ -1626,10 +1628,78 @@ static int qcow2_create2(const char *filename, int64_t total_size,
> >>>      Error *local_err = NULL;
> >>>      int ret;
> >>>+    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_METADATA) {
> >>>+        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);
> >>Hm, you could use the function minimal_blob_size() from my
> >>"qemu-img: Implement commit like QMP" series. It calculates the
> >>minimal size required for an L1 table, the refcount table and all
> >>refcount blocks for an image with a given number of guest sectors
> >>and a given number of "overhead", which are clusters that should be
> >>covered by the refcount structures in addition to
> >>L1+Reftable+Refblocks. To use it here, you would have to use it
> >>probably like this:
> >>
> >>meta_size = (1 + minimal_blob_size(total_size / BDRV_SECTOR_SIZE,
> >>cluster_bits, cluster_bits - BDRV_SECTOR_BITS, 1 + l2_clusters +
> >>(total_size >> cluster_bits)) + l2_clusters) << cluster_bits;
> >>
> >>The first parameter is the number of guest sectors; the second is
> >>cluster_bits; the third is parameter x, so that (1 << x) == number
> >>of sectors per cluster; and the fourth is the number of clusters
> >>which should be covered by the refcount structures in addition to
> >>L1+Reftable+Refblocks. These are the image header (1 cluster), the
> >>L2 tables (l2_clusters, which you'd have to calculate yourself (as
> >>you are already doing in this patch)) and the clusters for guest
> >>data (total_size >> cluster_bits).
> >>
> >>The function then returns the number of clusters required from
> >>L1+Reftable+Refblocks to which the image header cluster and the L2
> >>clusters have to be added to get the full metadata size.
> >>
> >>I'm suggesting you use this function (even though it's not merged
> >>yet, but fully reviewed), because I guess it's always likely to have
> >>some kind of small mistake in the size calculation for the refcount
> >>structures; and therefore it's better to have a single place where
> >>these mistakes can occur.
> >Thanks! I used minimal_blob_size() from your patch. But tests show that
> >the calculated meta size (and the image file size) is larger than the
> >number calculated from my patch and without patch(file size).
> >
> >test command:
> >qemu-img create -f qcow2 -o preallocation=metadata delme.img 8588490000
> >
> >result:
> >
> >without patch:
> >
> >Formatting 'delme.img', fmt=qcow2 size=8588490000 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off
> >-rw-r--r-- 1 root root 8590065664 Jun 20 15:02 delme.img
> >
> >
> >my patch:
> >
> >Formatting 'delme.img', fmt=qcow2 size=8588490000 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off
> >meta_size: 1507328, 1507328
> >total_size: 8588492800, 8588492800
> >-rw-r--r-- 1 root root 8590065664 Jun 20 15:01 delme.img
> >
> >
> >using minimal_blob_size:
> >
> >Formatting 'delme.img', fmt=qcow2 size=8588490000 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off
> >-rw-r--r-- 1 root root 8590131200 Jun 20 15:03 delme.img
> 
> Well, then I'll need to fix minimal_blob_size() anyway. ;-)
> 
> But, well, it's just one cluster too much, so I guess it's still
> fine (I mostly made sure to allocate enough space in any case
> without using (potentially imprecise) floating point operations; so
> it may indeed sometimes require too much space, but as long as it's
> rather insignificant, I guess we can live with it).

I'm fine with it. The only problem I'm not sure about is whether it will
bring any compatibility issues that the same cmd line gets two different
image sizes.

> 
> >I need to review minimal_blob_size() further.
> 
> In
> http://lists.nongnu.org/archive/html/qemu-devel/2014-04/msg04820.html
> I attached a document which explains how I ended up with my formula,
> maybe it'll prove helpful to you.

Thanks. looks OK to me.

> 
> Max
> 
> >
> >>
> >>As for your own calculation code: You should round all structure
> >>sizes separately to full clusters (the L1 table, the L2 tables, the
> >>refcount table and the refcount blocks). Especially this makes the
> >>refcount structure size so "hard" to calculate (or at least
> >>error-prone), as this has to be considered for the recursive
> >>calculation you demonstrated in your comment.
> >>
> >>>+
> >>>+        set_option_parameter_int(alloc_options, BLOCK_OPT_SIZE,
> >>>+                                 total_size + meta_size);
> >>Wouldn't it suffice for preallocation=metadata to only allocate meta_size?
> >>
> >>If we allocate total_size + meta_size for metadata preallocation as
> >>well, both metadata and full preallocation are the same (as far as I
> >>can see). But if we only allocate meta_size, we have to rework
> >>preallocate(): This function really "allocates" the data clusters
> >>from qcow2's point of view, which means they are holes in the raw
> >>file whereas all metadata should ideally be contained in the blob
> >>sized meta_size at the start of the file and the rest should be
> >>completely unused.
> >>
> >>With the current preallocate(), we either have to allocate
> >>everything (including data), or can't guarantee (and it will in fact
> >>never happen) that all metadata clusters fit into the meta_size
> >>sized blob, because it creates a mixture of interleaved data and
> >>metadata. The only way I can think of to really fix this is to
> >>rework preallocate() so that it doesn't use the regular
> >>qcow2_alloc_cluster_offset() function anymore but fills all metadata
> >>tables by itself (which should not be too hard for a simple linear
> >>preallocation of everything) and places all data clusters behind the
> >>metadata blob.
> >>
> >>>+        if (prealloc == PREALLOC_MODE_FULL) {
> >>>+            set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC, "full");
> >>>+        } else if (prealloc == PREALLOC_MODE_METADATA) {
> >>>+            set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC, "metadata");
> >>>+        }
> >>How about set_option_parameter(alloc_options, BLOCK_OPT_PREALLOC,
> >>PreallocMode_lookup[prealloc]); ?
> >>
> >>Max
> >>
> >>>+
> >>>+        options = alloc_options;
> >>>+    }
> >>>+
> >>>      ret = bdrv_create_file(filename, options, &local_err);
> >>>      if (ret < 0) {
> >>>          error_propagate(errp, local_err);
> >>>-        return ret;
> >>>+        goto out_options;
> >>>      }
> >>>      bs = NULL;
> >>>@@ -1637,7 +1707,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
> >>>                      NULL, &local_err);
> >>>      if (ret < 0) {
> >>>          error_propagate(errp, local_err);
> >>>-        return ret;
> >>>+        goto out_options;
> >>>      }
> >>>      /* Write the header */
> >>>@@ -1759,6 +1829,8 @@ out:
> >>>      if (bs) {
> >>>          bdrv_unref(bs);
> >>>      }
> >>>+out_options:
> >>>+    free_option_parameters(alloc_options);
> >>>      return ret;
> >>>  }
> >>>@@ -1790,13 +1862,11 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
> >>>                  cluster_size = options->value.n;
> >>>              }
> >>>          } 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, "metadata")) {
> >>>-                prealloc = PREALLOC_MODE_METADATA;
> >>>-            } else {
> >>>-                error_setg(errp, "Invalid preallocation mode: '%s'",
> >>>-                           options->value.s);
> >>>+            prealloc = qapi_enum_parse(PreallocMode_lookup, options->value.s,
> >>>+                                       PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
> >>>+                                       &local_err);
> >>>+            if (local_err) {
> >>>+                error_propagate(errp, local_err);
> >>>                  return -EINVAL;
> >>>              }
> >>>          } else if (!strcmp(options->name, BLOCK_OPT_COMPAT_LEVEL)) {
> >>>@@ -2359,7 +2429,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,
> >>>diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
> >>>index 28309a0..5689802 100644
> >>>--- a/tests/qemu-iotests/082.out
> >>>+++ b/tests/qemu-iotests/082.out
> >>>@@ -64,7 +64,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: create -f qcow2 -o ? TEST_DIR/t.qcow2 128M
> >>>@@ -75,7 +75,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: create -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 128M
> >>>@@ -86,7 +86,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: create -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 128M
> >>>@@ -97,7 +97,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: create -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 128M
> >>>@@ -108,7 +108,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: create -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 128M
> >>>@@ -119,7 +119,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: create -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 128M
> >>>@@ -130,7 +130,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: create -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 128M
> >>>@@ -141,7 +141,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 128M
> >>>@@ -167,7 +167,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: create -o help
> >>>@@ -245,7 +245,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: convert -O qcow2 -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >>>@@ -256,7 +256,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: convert -O qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >>>@@ -267,7 +267,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: convert -O qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >>>@@ -278,7 +278,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: convert -O qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >>>@@ -289,7 +289,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: convert -O qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >>>@@ -300,7 +300,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: convert -O qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >>>@@ -311,7 +311,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: convert -O qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >>>@@ -322,7 +322,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base
> >>>@@ -348,7 +348,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: convert -o help
> >>>@@ -415,7 +415,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: amend -f qcow2 -o ? TEST_DIR/t.qcow2
> >>>@@ -426,7 +426,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: amend -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2
> >>>@@ -437,7 +437,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: amend -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2
> >>>@@ -448,7 +448,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: amend -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2
> >>>@@ -459,7 +459,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: amend -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2
> >>>@@ -470,7 +470,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: amend -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2
> >>>@@ -481,7 +481,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: amend -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2
> >>>@@ -492,7 +492,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2
> >>>@@ -520,7 +520,7 @@ backing_file     File name of a base image
> >>>  backing_fmt      Image format of the base image
> >>>  encryption       Encrypt the image
> >>>  cluster_size     qcow2 cluster size
> >>>-preallocation    Preallocation mode (allowed values: off, metadata)
> >>>+preallocation    Preallocation mode (allowed values: off, metadata, full)
> >>>  lazy_refcounts   Postpone refcount updates
> >>>  Testing: convert -o help