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

[Max Reitz <mreitz@redhat.com>]

On 29.08.2014 10:33, 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 | 92 +++++++++++++++++++++++++++++++++++++++++++------------
>   qemu-doc.texi     |  8 +++++
>   qemu-img.texi     |  8 +++++
>   3 files changed, 88 insertions(+), 20 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index abe0759..25f66f2 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>
> @@ -1365,6 +1366,9 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
>       int result = 0;
>       int64_t total_size = 0;
>       bool nocow = false;
> +    PreallocMode prealloc = PREALLOC_MODE_OFF;
> +    char *buf = NULL;
> +    Error *local_err = NULL;
>   
>       strstart(filename, "file:", &filename);
>   
> @@ -1372,37 +1376,80 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
>       total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
>                             BDRV_SECTOR_SIZE);
>       nocow = qemu_opt_get_bool(opts, BLOCK_OPT_NOCOW, false);
> +    buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> +    prealloc = qapi_enum_parse(PreallocMode_lookup, buf,
> +                               PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
> +                               &local_err);
> +    g_free(buf);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        result = -EINVAL;
> +        goto out;
> +    }
>   
>       fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
>                      0644);
>       if (fd < 0) {
>           result = -errno;
>           error_setg_errno(errp, -result, "Could not create file");
> -    } else {
> -        if (nocow) {
> +        goto out;
> +    }
> +
> +    if (nocow) {
>   #ifdef __linux__
> -            /* Set NOCOW flag to solve performance issue on fs like btrfs.
> -             * This is an optimisation. The FS_IOC_SETFLAGS ioctl return value
> -             * will be ignored since any failure of this operation should not
> -             * block the left work.
> -             */
> -            int attr;
> -            if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) {
> -                attr |= FS_NOCOW_FL;
> -                ioctl(fd, FS_IOC_SETFLAGS, &attr);
> -            }
> -#endif
> +        /* Set NOCOW flag to solve performance issue on fs like btrfs.
> +         * This is an optimisation. The FS_IOC_SETFLAGS ioctl return value
> +         * will be ignored since any failure of this operation should not
> +         * block the left work.
> +         */
> +        int attr;
> +        if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) {
> +            attr |= FS_NOCOW_FL;
> +            ioctl(fd, FS_IOC_SETFLAGS, &attr);
>           }
> +#endif
> +    }
>   
> -        if (ftruncate(fd, total_size) != 0) {
> -            result = -errno;
> -            error_setg_errno(errp, -result, "Could not resize file");
> -        }
> -        if (qemu_close(fd) != 0) {
> -            result = -errno;
> -            error_setg_errno(errp, -result, "Could not close the new file");
> +    if (ftruncate(fd, total_size) != 0) {
> +        result = -errno;
> +        error_setg_errno(errp, -result, "Could not resize file");
> +        goto out_close;
> +    }
> +
> +    if (prealloc == PREALLOC_MODE_FULL) {
> +        /* posix_fallocate() doesn't set errno. */
> +        result = -posix_fallocate(fd, 0, total_size);
> +        if (result != 0) {
> +            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);
>           }
> +    } else if (prealloc != PREALLOC_MODE_OFF) {
> +        result = -1;

As for qcow2 in patch 4, I'd prefer -EINVAL.

> +        error_setg(errp, "Unsupported preallocation mode: %s",
> +                   PreallocMode_lookup[prealloc]);
> +    }
> +
> +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;
>   }
>   
> @@ -1585,6 +1632,11 @@ static QemuOptsList raw_create_opts = {
>               .type = QEMU_OPT_BOOL,
>               .help = "Turn off copy-on-write (valid only on btrfs)"
>           },
> +        {
> +            .name = BLOCK_OPT_PREALLOC,
> +            .type = QEMU_OPT_STRING,
> +            .help = "Preallocation mode (allowed values: off, full)"
> +        },
>           { /* end of list */ }
>       }
>   };
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index 2b232ae..2637765 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -527,6 +527,14 @@ Linux or NTFS on Windows), then only the written sectors will reserve
>   space. Use @code{qemu-img info} to know the real size used by the
>   image or @code{ls -ls} on Unix/Linux.
>   
> +Supported options:
> +@table @code
> +@item preallocation
> +Preallocation mode(allowed values: @code{off}, @code{full}). An image is

Missing space in front of the opening bracket.

> +fully preallocated by calling posix_fallocate() if it's available, or by

Hm, I personally am not so happy about contractions ("it's") in 
persistent documentation (even source code comments). Although I know 
there are already some of them in qemu-doc.texi, I'd rather avoid them. 
But I'll leave this up to you as I'm no native speaker.

> +writing zeros to underlying storage.
> +@end table
> +
>   @item qcow2
>   QEMU image format, the most versatile format. Use it to have smaller
>   images (useful if your filesystem does not supports holes, for example
> diff --git a/qemu-img.texi b/qemu-img.texi
> index cb68948..063ec61 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -418,6 +418,14 @@ Linux or NTFS on Windows), then only the written sectors will reserve
>   space. Use @code{qemu-img info} to know the real size used by the
>   image or @code{ls -ls} on Unix/Linux.
>   
> +Supported options:
> +@table @code
> +@item preallocation
> +Preallocation mode(allowed values: @code{off}, @code{full}). An image is
> +fully preallocated by calling posix_fallocate() if it's available, or by
> +writing zeros to underlying storage.
> +@end table
> +

Same as for qemu-doc.texi.

However, these are all minor, so with "result = -EINVAL" and the missing 
space added before the brackets:

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

>   @item qcow2
>   QEMU image format, the most versatile format. Use it to have smaller
>   images (useful if your filesystem does not supports holes, for example