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

[Stefan Weil <weil@mail.berlios.de>]

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

Stefan