Re: [Qemu-devel] Review of ways to create BDSes (was: Review of monitor commands identifying BDS / BB by name)

[Kevin Wolf]

Am 18.12.2014 um 16:25 hat Markus Armbruster geschrieben:
> = Introduction =
> 
> BDSes can be opened in various ways.  Some of them don't provide for
> user configuration.  Some of them should.
> 
> Example: -drive format=qcow2,file=foo.qcow2 where foo.qcow2 has a raw
> backing file foo.raw.  This creates the the following tree of BDSes:
> 
>             (qcow2,foo.qcow2)
>               /            \
>     (file,foo.qcow2)    (raw,foo.raw)
>                             |
>                        (file,foo.raw)
> 
> Before Kevin added driver-specific options, -drive let you configure
> basically just the root.  Configuration for the others was inferred from
> the root's configuration and the images.  Driver-specific options let
> you configure all the nodes.  Defaults are still inferred as before.
> 
> Example: blockdev-snapshot-sync provides only a small subset of the full
> configuration options for the BDS it creates.  Could be fixed by
> duplicating the full options, i.e. blockdev-add's.  But a command that
> just snapshots and leaves BDS creation to blockdev-add would be cleaner.
> 
> This is a systematic review of all the ways you can open BDSes in qemu
> proper, i.e. not in qemu-{img,io,nbd}.  I tracked them down by following
> the call chains leading to bdrv_open().

Possibly also interesting for a followup: All the ways you can reopen
BDSes with different options/flags.

> = QMP Commands =
> 
> * block-commit
> 
>   I figure this clones the @base BDS initially, and replaces it by the
>   clone finally.  Is support for user configuration for this clone
>   wanted?

I don't think such a clone exists. Data is directly committed to @base.
The command looks to me as if it could be okay.

> * blockdev-add
> 
>   Boils down to a bdrv_open(), which is discussed in the next section.
> 
> * blockdev-snapshot-sync
> 
>   Creates a new image and a new BDS with only a few configuration
>   options: @snapshot-file (the filename), @snapshot-node-name,
>   @format=qcow2, @mode.  Conflates three operations: create image, open
>   image, snapshot.  I guess we want to replace it by a basic snapshot
>   operation that can be used with with blockdev-add and some command to
>   create images.

Yes. We should have called this one drive-snapshot, it fits better in
the drive-* family of commands. We can call the real blockdev-style
command blockdev-snapshot - it is still synchronous technically, but it
doesn't do anything like bdrv_open() that could be blocking.

> * change
> 
>   Closes, then opens a BDS with just two configuration options: @target
>   (the filename) and @arg (the format).  Needs replacing.

Max (added to CC) is working on it.

> * drive-backup
> 
>   Similar to blockdev-snapshot-sync, except the filename is called
>   @target, and the node name can't be configured.  I guess we want to
>   replace it by a basic backup operation.
> 
> * drive-mirror
> 
>   Similar to blockdev-snapshot-sync, except the filename is called
>   @target, and the node name @node-name.  I guess we want to replace it
>   by a basic mirror operation.

Yes. We called these drive-* instead of blockdev-* intentionally, so
that the latter names would be free for operations working on existing
BDSes.

> * transaction
> 
>   This is a wrapper around a list of transaction-capable commands.
>   Thus, nothing new here.
> 
> 
> = Generic block layer =
> 
> bdrv_open() opens a BDS and possibly children "file" and "backing"
> according to its configuration.
> 
> Subtypes of BlockdevOptionsGenericFormat have configuration for "file".
> 
> Subtypes of BlockdevOptionsGenericCOWFormat additionally have
> configuration for "backing" (defaults to "infer from COW image").
> 
> bdrv_open() can additionally splice in a QCOW2 BDS to implement
> snapshot=on.  No way to configure, but that's okay, because it's a
> convenience feature, and to configure you simply do the splicing
> explicitly.
> 
> 
> = Block driver methods =
> 
> == bdrv_create() ==
> 
> The BDSes created here are all internal temporaries of the method, hence
> user configuration isn't needed.  Correct?

Filenames ought to be enough for everyone. Not.

But at the moment all the callers can't deal with non-filename
specifications of the image location, so that's a larger problem.

> == bdrv_file_open() ==
> 
> * quorum_open()
> 
>   Creates / connects to its children according to configuration in
>   BlockdevOptionsQuorum.
> 
> * blkdebug_open()
> 
>   Creates / connects to its children according to configuration in
>   BlockdevOptionsBlkdebug.
> 
> * blkverify_open()
> 
>   Creates / connects to its children according to configuration in
>   BlockdevOptionsBlkverify.
> 
> * vvfat's enable_write_target()
> 
>   You don't want to know.

This would have been an interesting one for a change. ;-)

> == bdrv_open() ==
> 
> * vmdk_open()
> 
>   Creates BDSes for its extents, configuration inferred from images.
>   Looks like this needs work.

Very much so.

> = Xen =
> 
> blk_connect() calls bdrv_open() under a /* setup via xenbus */ heading.
> Looks like backward compatibility crap to me.

Are you sure? But Xen is another "you don't want to know" for me.

Kevin