Re: [Qemu-devel] [PATCH RFC v11 6/6] qcow2: Add falloc and full preallocation option

[Max Reitz <mreitz@redhat.com>]

On 02.07.2014 10:17, Hu Tao wrote:
> This adds preallocation=falloc and preallocation=full mode to qcow2
> image creation.
>
> preallocation=full allocates disk space by writing zeros to disk to
> ensure disk space in any cases.
>
> preallocation=falloc likes preallocation=full, but allocates disk space
> by posix_fallocate().
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>   block/qcow2.c              | 29 ++++++++++++++++++++++++-
>   tests/qemu-iotests/082.out | 54 +++++++++++++++++++++++-----------------------
>   2 files changed, 55 insertions(+), 28 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index cfba93b..f2df8cb 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1593,6 +1593,9 @@ static int preallocate(BlockDriverState *bs)
>       return 0;
>   }
>   
> +static uint64_t minimal_blob_size(uint64_t ts, int cb, int spcb,
> +                                  uint64_t overhead);
> +
>   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, PreallocMode prealloc,
> @@ -1628,6 +1631,29 @@ 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_FALLOC) {
> +        int64_t meta_size = 0;
> +        unsigned nl2e;
> +
> +        total_size = align_offset(total_size, cluster_size);
> +
> +        /* total size of L2 tables */
> +        nl2e = total_size / cluster_size;

This might overflow, better use a uint64_t for nl2e. Also: You're using 
">> cluster_bits" below to divide by the cluster size; I'd prefer using 
the same method in both places (either ">> cluster_bits" or "/ 
cluster_size").

> +        nl2e = align_offset(nl2e, cluster_size / sizeof(uint64_t));
> +        uint64_t l2_clusters = nl2e * sizeof(uint64_t) >> cluster_bits;
> +
> +        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;
> +
> +        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size + meta_size);
> +        qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_lookup[prealloc]);
> +    }
> +
>       ret = bdrv_create_file(filename, opts, &local_err);
>       if (ret < 0) {
>           error_propagate(errp, local_err);

Maybe I'd additionally change the "if (prealloc)" later in 
qcow2_create2() to "if (prealloc != PREALLOC_MODE_NONE)" to make it more 
clear that there is more than just one method of preallocation now.


Maybe we also want to modify preallocate() later on to allocate all 
metadata structures in a single block (because, who knows, we might want 
a linear layout for the data clusters for some strange reason), but 
currently there is no need for that.

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