From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46068) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WzgLp-0004ek-6g for qemu-devel@nongnu.org; Wed, 25 Jun 2014 02:06:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WzgLg-0006jD-UY for qemu-devel@nongnu.org; Wed, 25 Jun 2014 02:06:33 -0400 Received: from [59.151.112.132] (port=54623 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WzgLg-0006iv-1v for qemu-devel@nongnu.org; Wed, 25 Jun 2014 02:06:24 -0400 Date: Wed, 25 Jun 2014 14:04:25 +0800 From: Hu Tao Message-ID: <20140625060424.GD27669@G08FNSTD100614.fnst.cn.fujitsu.com> References: <539CA4B6.5070505@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline In-Reply-To: <539CA4B6.5070505@redhat.com> Subject: Re: [Qemu-devel] [PATCH v10 5/6] raw-posix: Add full image preallocation option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: kwolf@redhat.com, Peter Lieven , qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi , y-goto@jp.fujitsu.com On Sat, Jun 14, 2014 at 09:38:30PM +0200, Max Reitz wrote: > 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 > >--- > > 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 > >@@ -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. qcow2's behaviour of metadata preallocation depends on raw's, this is why I chose the latter here. > > 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. The difference is posix_fallocate() reserves space but ftruncate() doesn't, as said in man page: After a successful call to posix_fallocate(), subsequent writes to bytes in the specified range are guaranteed not to fail because of lack of disk space. however, posix_fallocate() doesn't guarantee this in other cases like thin provisioning, this is why preallocation=metadata is different than preallocation=full. Hu