From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59241) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOvtj-00058a-KG for qemu-devel@nongnu.org; Tue, 02 Sep 2014 17:46:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XOvte-0004Y6-7f for qemu-devel@nongnu.org; Tue, 02 Sep 2014 17:45:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:8390) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOvtd-0004Xy-Q5 for qemu-devel@nongnu.org; Tue, 02 Sep 2014 17:45:50 -0400 Message-ID: <54063A82.9050006@redhat.com> Date: Tue, 02 Sep 2014 23:45:38 +0200 From: Max Reitz MIME-Version: 1.0 References: <67d97abc587e7c6985166dbe800686938ac8adb5.1409299732.git.hutao@cn.fujitsu.com> In-Reply-To: <67d97abc587e7c6985166dbe800686938ac8adb5.1409299732.git.hutao@cn.fujitsu.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v13 5/6] raw-posix: Add full preallocation option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hu Tao , qemu-devel@nongnu.org Cc: Kevin Wolf , Fam Zheng , "Richard W.M. Jones" , Stefan Hajnoczi , Yasunori Goto 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 > --- > 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 > @@ -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 > @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