From patchwork Wed May 5 12:45:05 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Wolf X-Patchwork-Id: 51714 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id B8CE4B7D61 for ; Thu, 6 May 2010 04:37:44 +1000 (EST) Received: from localhost ([127.0.0.1]:50285 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O9jRa-0003Ya-T4 for incoming@patchwork.ozlabs.org; Wed, 05 May 2010 14:35:39 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O9dzF-00037Y-1J for qemu-devel@nongnu.org; Wed, 05 May 2010 08:46:01 -0400 Received: from [140.186.70.92] (port=53063 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O9dz9-0002zh-J6 for qemu-devel@nongnu.org; Wed, 05 May 2010 08:46:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O9dyy-0002YI-Sa for qemu-devel@nongnu.org; Wed, 05 May 2010 08:45:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:14202) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O9dyw-0002Xm-9z for qemu-devel@nongnu.org; Wed, 05 May 2010 08:45:44 -0400 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o45CjV8H032630 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 5 May 2010 08:45:32 -0400 Received: from dhcp-5-188.str.redhat.com (dhcp-5-217.str.redhat.com [10.32.5.217]) by int-mx05.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o45CjSTb003277 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 5 May 2010 08:45:30 -0400 Message-ID: <4BE16851.2040603@redhat.com> Date: Wed, 05 May 2010 14:45:05 +0200 From: Kevin Wolf User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100330 Fedora/3.0.4-1.fc12 Thunderbird/3.0.4 MIME-Version: 1.0 To: Stefan Weil Subject: Re: [Qemu-devel] [PATCH, RFC] block: separate raw images from the file protocol References: <20100407203024.GA30897@lst.de> <4BBDA6DE.5020306@redhat.com> <4BE08A7B.9030804@mail.berlios.de> In-Reply-To: <4BE08A7B.9030804@mail.berlios.de> X-Scanned-By: MIMEDefang 2.67 on 10.5.11.18 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. Cc: Christoph Hellwig , qemu-devel@nongnu.org X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Am 04.05.2010 22:58, schrieb Stefan Weil: > 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 Does the attached patch fix the problem? Kevin diff --git a/block.c b/block.c index 48305b7..1db8473 100644 --- a/block.c +++ b/block.c @@ -290,13 +290,13 @@ static BlockDriver *find_protocol(const char *filename) /* TODO Drivers without bdrv_file_open must be specified explicitly */ + p = strchr(filename, ':'); + if (!p || #ifdef _WIN32 - if (is_windows_drive(filename) || + is_windows_drive(filename) || is_windows_drive_prefix(filename)) - return bdrv_find_format("file"); #endif - p = strchr(filename, ':'); - if (!p) { + { drv1 = find_hdev_driver(filename); if (!drv1) { drv1 = bdrv_find_format("file");