From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MVL9l-0001Vp-Kb for qemu-devel@nongnu.org; Mon, 27 Jul 2009 04:02:01 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MVL9h-0001Rt-QD for qemu-devel@nongnu.org; Mon, 27 Jul 2009 04:02:01 -0400 Received: from [199.232.76.173] (port=39469 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MVL9h-0001Rn-ON for qemu-devel@nongnu.org; Mon, 27 Jul 2009 04:01:57 -0400 Received: from mx20.gnu.org ([199.232.41.8]:14397) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MVL9h-00062D-2K for qemu-devel@nongnu.org; Mon, 27 Jul 2009 04:01:57 -0400 Received: from mx2.redhat.com ([66.187.237.31]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MVL9f-0008DF-Ef for qemu-devel@nongnu.org; Mon, 27 Jul 2009 04:01:55 -0400 Message-ID: <4A6D5EA2.2080706@redhat.com> Date: Mon, 27 Jul 2009 10:00:34 +0200 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Add new block driver for the VDI format (use aio) References: <4A6888AC.3050509@mail.berlios.de> <1248380985-7362-1-git-send-email-weil@mail.berlios.de> <4A697C7E.80400@redhat.com> <4A69DF48.7000109@mail.berlios.de> In-Reply-To: <4A69DF48.7000109@mail.berlios.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Weil Cc: QEMU Developers , Christoph Hellwig Stefan Weil schrieb: >>> +/* Enable (currently) unsupported features (not implemented yet). */ >>> +//~ #define CONFIG_VDI_UNSUPPORTED >>> + >>> +/* Support non-standard block (cluster) size. */ >>> +//~ #define CONFIG_VDI_BLOCK_SIZE >>> >> Actually, this is only about support for image creation. Any reason why >> we shouldn't support creating images with non-standard block sizes? The >> code already supports opening such images unconditionally, so the only >> effect of turning it off for image creation is that we can't test that >> functionality in qemu-iotests. >> >> [Oh, sorry, actually there is a check in open which I missed at first. >> Any reason why we can't support it? But it's consistent at least.] >> > > Multiples of 512 (SECTOR_SIZE) might work. > > VirtualBox uses 1 MiB blocks, and I did not see options to create images > with different block sizes. Maybe they even don't support such images. > So I did not spend the time to test other block sizes. > Why implement things nobody needs? Ok, that makes sense. Probably we should remove the #define completely then. I mean, why creating images that nobody - not even we ourselves - can read? >> >>> +/* Support static (pre-allocated) images. */ >>> +#define CONFIG_VDI_STATIC_IMAGE >>> + >>> +/* Command line option for static images. */ >>> +#define BLOCK_OPT_STATIC "static" >>> >> What about calling it "preallocate" and moving it to block_int.h? I >> think this could make sense for other drivers, too. >> > > Yes, this would be reasonable if we had more drivers with support > for "preallocate". > > The VDI documentation calls these images "static", and they prefer > dynamic images, so this static option is not really very important. I might consider implementing it for qcow2. Cluster allocation is the really slow part, so having complete L1/L2 tables in place from the very beginning could speed up things. Though I guess that for static images typically not only metadata is preallocated, but zeros are written for the whole disk content? Maybe we could implement a three-way flag like preallocate=[no,metadata,data] and let qemu-img handle the data part (writing zeros is the same for all formats and would even work with raw). >>> +static int vdi_make_empty(BlockDriverState *bs) >>> +{ >>> + /* TODO: missing code. */ >>> + logout("\n"); >>> + return 0; >>> +} >>> >> If you don't implement it, leave it out. Setting >> bdrv_vdi.bdrv_make_empty != NULL means that you claim to have that >> functionality. >> >> > > I did not analyse what *_make_empty is supposed to do. > This is one of the details were hints of the block driver experts > would be helpful. It's used after committing to a backing file. qcow1 seems to be the only format actually implementing it. It complete clears the L1/L2 tables (= the block map for VDI) so that all accesses go to the backing file again and it can shrink the image file. >>> + >>> +#if defined(CONFIG_AIO) >>> + >>> +#if 0 >>> >> I guess you should remove this block before the patch is included. >> > > This is also one of the details were hints of the block driver experts > would be helpful as I did not understand this aio_remove / aio_cancel > mechanism. I wouldn't consider myself an AIO expert and I don't want to tell you something wrong, so maybe Christoph would be the right one here? >> Does VDI support compression even theoretically? >> > > I think it would be possible to extend the specification > to support compression or encryption. > > The official specification (as far as I know it) does not > support compression (nor encryption). Then remove the entry. The function vdi_write_compressed doesn't exist and doesn't even make sense with the current specification. The same applies for vdi_set_key. Kevin