From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42281) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2ne7-0000F4-Ly for qemu-devel@nongnu.org; Thu, 03 Jul 2014 16:30:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X2ne3-0005QG-1T for qemu-devel@nongnu.org; Thu, 03 Jul 2014 16:30:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54701) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2ne2-0005QA-P2 for qemu-devel@nongnu.org; Thu, 03 Jul 2014 16:30:14 -0400 Message-ID: <53B5BD4E.7090605@redhat.com> Date: Thu, 03 Jul 2014 22:30:06 +0200 From: Max Reitz MIME-Version: 1.0 References: <6583fb5f97a6d173be3865d2794ceb3cccf7c013.1404287261.git.hutao@cn.fujitsu.com> In-Reply-To: <6583fb5f97a6d173be3865d2794ceb3cccf7c013.1404287261.git.hutao@cn.fujitsu.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC v11 6/6] qcow2: Add falloc and full preallocation option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hu Tao , Kevin Wolf Cc: Yasunori Goto , qemu-devel@nongnu.org, Stefan Hajnoczi On 02.07.2014 10:17, Hu Tao wrote: > This adds preallocation=falloc and preallocation=full mode to qcow2 > image creation. > > preallocation=full allocates disk space by writing zeros to disk to > ensure disk space in any cases. > > preallocation=falloc likes preallocation=full, but allocates disk space > by posix_fallocate(). > > Signed-off-by: Hu Tao > --- > block/qcow2.c | 29 ++++++++++++++++++++++++- > tests/qemu-iotests/082.out | 54 +++++++++++++++++++++++----------------------- > 2 files changed, 55 insertions(+), 28 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index cfba93b..f2df8cb 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1593,6 +1593,9 @@ static int preallocate(BlockDriverState *bs) > return 0; > } > > +static uint64_t minimal_blob_size(uint64_t ts, int cb, int spcb, > + uint64_t overhead); > + > static int qcow2_create2(const char *filename, int64_t total_size, > const char *backing_file, const char *backing_format, > int flags, size_t cluster_size, PreallocMode prealloc, > @@ -1628,6 +1631,29 @@ static int qcow2_create2(const char *filename, int64_t total_size, > Error *local_err = NULL; > int ret; > > + if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) { > + int64_t meta_size = 0; > + unsigned nl2e; > + > + total_size = align_offset(total_size, cluster_size); > + > + /* total size of L2 tables */ > + nl2e = total_size / cluster_size; This might overflow, better use a uint64_t for nl2e. Also: You're using ">> cluster_bits" below to divide by the cluster size; I'd prefer using the same method in both places (either ">> cluster_bits" or "/ cluster_size"). > + nl2e = align_offset(nl2e, cluster_size / sizeof(uint64_t)); > + uint64_t l2_clusters = nl2e * sizeof(uint64_t) >> cluster_bits; > + > + meta_size = > + (1 + > + minimal_blob_size(total_size / BDRV_SECTOR_SIZE, > + cluster_bits, cluster_bits - BDRV_SECTOR_BITS, > + 1 + l2_clusters + > + (total_size >> cluster_bits)) + > + l2_clusters) << cluster_bits; > + > + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size + meta_size); > + qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_lookup[prealloc]); > + } > + > ret = bdrv_create_file(filename, opts, &local_err); > if (ret < 0) { > error_propagate(errp, local_err); Maybe I'd additionally change the "if (prealloc)" later in qcow2_create2() to "if (prealloc != PREALLOC_MODE_NONE)" to make it more clear that there is more than just one method of preallocation now. Maybe we also want to modify preallocate() later on to allocate all metadata structures in a single block (because, who knows, we might want a linear layout for the data clusters for some strange reason), but currently there is no need for that. Reviewed-by: Max Reitz