From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49161) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wvtmv-0000Cg-71 for qemu-devel@nongnu.org; Sat, 14 Jun 2014 15:38:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wvtmq-0001zh-Ic for qemu-devel@nongnu.org; Sat, 14 Jun 2014 15:38:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44705) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wvtmq-0001xu-9V for qemu-devel@nongnu.org; Sat, 14 Jun 2014 15:38:48 -0400 Message-ID: <539CA4B6.5070505@redhat.com> Date: Sat, 14 Jun 2014 21:38:30 +0200 From: Max Reitz MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Hu Tao , qemu-devel@nongnu.org Cc: kwolf@redhat.com, Peter Lieven , Markus Armbruster , Stefan Hajnoczi , y-goto@jp.fujitsu.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 > --- > 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. 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 } > }; >