On Tue, Sep 02, 2014 at 11:45:38PM +0200, Max Reitz wrote:
> 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 <hu...@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.
Okay.
>
> >+ 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.
Okay. I checked that in other places of opening bracket there is a space
before them.
>
> >+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.
I'm not, either. I don't know which one in "it's" and "it is" is more
common, but I can change the contraction if it makes you feel better:)
Regards,
Hu
>
> >+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 <mre...@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
