block: separate raw images from the file protocol

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.

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.

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.

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.

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 <[email protected]>

Index: qemu/Makefile.objs
===================================================================

Am 07.04.2010 22:30, schrieb Christoph Hellwig:

Quoted 9 lines

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.

Quoted 10 lines

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.

Quoted 8 lines

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.

Quoted 9 lines

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.

Quoted 11 lines

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<[email protected]>

Index: qemu/Makefile.objs
===================================================================

Makefile.objs block-obj-y += nbd.o block.o aio.o aes.o · line 18

Quoted 8 lines

Makefile.objs +1 -1

@@ -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?

Quoted 4 lines

Makefile.objs +0 -0

@@ -1026,12 +985,12 @@ static int hdev_create(const char *filen
 
  static BlockDriver bdrv_host_device = {
      .format_name        = "host_device",

Makefile.objs static int hdev_create(const char *filen · line 996

Quoted 10 lines

Makefile.objs +1 -1

+    .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

Am 08.04.2010 11:50, schrieb Kevin Wolf:

Quoted 118 lines

Am 07.04.2010 22:30, schrieb Christoph Hellwig:

Quoted 9 lines

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.

Quoted 10 lines

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.

Quoted 8 lines

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.

Quoted 9 lines

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.

Quoted 11 lines

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<[email protected]>

Index: qemu/Makefile.objs
===================================================================

Makefile.objs block-obj-y += nbd.o block.o aio.o aes.o · line 18

Quoted 8 lines

Makefile.objs +1 -1

@@ -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?

Quoted 4 lines

Makefile.objs +0 -0

@@ -1026,12 +985,12 @@ static int hdev_create(const char *filen
 
  static BlockDriver bdrv_host_device = {
      .format_name        = "host_device",

Makefile.objs static int hdev_create(const char *filen · line 996

Quoted 10 lines

Makefile.objs +1 -1

+    .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.

Quoted 2 lines

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

Am 04.05.2010 22:58, schrieb Stefan Weil:

Quoted 6 lines

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