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

[Max Reitz <mreitz@redhat.com>]

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 }
>   };
>