From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O9PFH-0008Q2-7s for qemu-devel@nongnu.org; Tue, 04 May 2010 17:01:35 -0400 Received: from [140.186.70.92] (port=41871 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O9PEn-0005XH-NA for qemu-devel@nongnu.org; Tue, 04 May 2010 17:01:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O9PCk-0001P7-HP for qemu-devel@nongnu.org; Tue, 04 May 2010 17:01:05 -0400 Received: from moutng.kundenserver.de ([212.227.126.171]:59829) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O9PCj-0001OI-DG for qemu-devel@nongnu.org; Tue, 04 May 2010 16:58:58 -0400 Message-ID: <4BE08A7B.9030804@mail.berlios.de> Date: Tue, 04 May 2010 22:58:35 +0200 From: Stefan Weil MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH, RFC] block: separate raw images from the file protocol References: <20100407203024.GA30897@lst.de> <4BBDA6DE.5020306@redhat.com> In-Reply-To: <4BBDA6DE.5020306@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Christoph Hellwig , qemu-devel@nongnu.org Am 08.04.2010 11:50, schrieb Kevin Wolf: > Am 07.04.2010 22:30, schrieb Christoph Hellwig: > >> We're running into various problems because the "raw" file access, which >> is used internally by the various image formats is entangled with the >> "raw" image format, which maps the VM view 1:1 to a file system. >> >> This patch renames the raw file backends to the file protocol which >> is treated like other protocols (e.g. nbd and http) and adds a new >> "raw" image format which is just a wrapper around calls to the underlying >> protocol. >> > As you know and as I mentioned in previous discussions this approach is > exactly what I think we need in the block layer. > > You provided a nice long patch description that covers almost > everything, so I think I can put the greatest part of my comments there. > > >> The patch is surprisingly simple, besides changing the probing logical >> in block.c to only look for image formats when using bdrv_open and >> renaming of the old raw protocols to file there's almost nothing in there. >> >> One thing that looks suspicious in the patch is moving the actual >> posix file creation from raw-posix into the new raw image. This is >> a layering violation, but exactly the same as done by all other image >> formats implementing the create operations, and not easily fixable >> without a major API change in this area. >> > This is not only a layering violation, but also buggy in this case. > raw-win32.c has a different implementation of raw_create which wouldn't > be called any more. > > The two solutions that I see are making raw_create a wrapper that calls > the create function of the protocol, or do make the step and use bdrv_* > in the create functions of the drivers. I think the former is what could > be done to keep this patch simple, but the latter is what we should aim > for longer term. > > >> The only issues still open are in the handling of the host devices. >> Firstly in current qemu we can specifiy the host* format names >> on various command line acceping images, but the new code can't >> do that without adding some translation. Second the layering breaks >> the no_zero_init flag in the BlockDriver used by qemu-img. I'm not >> happy how this is done per-driver instead of per-state so I'll >> prepare a separate patch to clean this up. >> > Hm, I don't like that very much, but there's probably no sane way around > it. It's clearly a property of the protocol and not of a single device, > but protocols might be stacked and just checking the first one doesn't > give the right result. > > Anyway, before merging this patch we obviously need to fix this kind of > things (is it caught by qemu-iotests, by the way?). I'm not sure if we > should add a compatibility translation of host_device => raw or if we > should just remove support for that completely. It would be helpful to > know if this is actually used. > > >> There's some more cleanup opportunity after this patch, e.g. using >> separate lists and registration functions for image formats vs >> protocols and maybe even host drivers, but this can be done at a >> later stage. >> >> Also there's a check for protocol in bdrv_open for the BDRV_O_SNAPSHOT >> case that I don't quite understand, but which I fear won't work as >> expected - possibly even before this patch. >> > You mean that is_protocol thing? It comes into play when you do strange > things like qemu -hda fat:/tmp/testdir -snapshot and I think it actually > does work. > > Hm, apropos vvfat... Should vvfat actually be implemented as raw backed > by vvfat now instead of using vvfat directly? We could then forbid > protocols to be used directly. > > >> Note that this patch requires various recent block patches from Kevin >> and me, which should all be in his block queue. >> >> Signed-off-by: Christoph Hellwig >> >> Index: qemu/Makefile.objs >> =================================================================== >> --- qemu.orig/Makefile.objs 2010-04-07 13:56:27.429254145 +0200 >> +++ qemu/Makefile.objs 2010-04-07 22:01:24.974284455 +0200 >> @@ -12,7 +12,7 @@ block-obj-y += nbd.o block.o aio.o aes.o >> block-obj-$(CONFIG_POSIX) += posix-aio-compat.o >> block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o >> >> -block-nested-y += cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vvfat.o >> +block-nested-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vvfat.o >> block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o >> block-nested-y += parallels.o nbd.o >> block-nested-$(CONFIG_WIN32) += raw-win32.o >> > This hunk only applies with fuzz on my queue (caused by blkdebug.o). Can > you make sure to rebase the final version on the queue? > > >> @@ -1026,12 +985,12 @@ static int hdev_create(const char *filen >> >> static BlockDriver bdrv_host_device = { >> .format_name = "host_device", >> + .protocol_name = "host_device", >> .instance_size = sizeof(BDRVRawState), >> .bdrv_probe_device = hdev_probe_device, >> .bdrv_open = hdev_open, >> .bdrv_close = raw_close, >> .bdrv_create = hdev_create, >> - .create_options = raw_create_options, >> .no_zero_init = 1, >> .bdrv_flush = raw_flush, >> > A driver that has a bdrv_create needs to also have create_options. > Either retain both or remove both. qemu-img create -f host_device > segfaults with this change. > > Kevin > This patch (commit 84a12e6648444f517055138a7d7f25a22d7e1029) breaks QEMU for Win32: QEMU can no longer access \\.\PhysicalDrive0 - a feature I use quite often. Found by git bisect, tested like this: qemu \\.\PhysicalDrive0 Stefan