Re: [Qemu-devel] [PATCH, RFC] block: separate raw images from the file protocol

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5989 bytes --]

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<hch@lst.de>
>>>
>>> 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

[-- Attachment #2: win_host_device.patch --]
[-- Type: text/x-patch, Size: 647 bytes --]

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");