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 <address@hidden>
> ---
>   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