Re: [Qemu-devel] Review of ways to create BDSes

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

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

Can do "all the ways you can reopen BDSes".  Finding out whether and how
options or flags can be changed in each case would be too much for one
go, I'm afraid.

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

Here's what made me speculate about clones.  Call chain:

    qmp_block_commit()
        commit_active_start()
            mirror_start_job(), passing commit_active_job_driver

On completion:

    block_job_complete()
        mirror_complete(), through commit_active_job_driver.complete
            bdrv_open_backing_file()
                bdrv_open()

I now see that my reading wasn't correct.  But I can't see offhand what
exactly happens here.  Can you enlighten me?

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

Yes.

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

Moreover, the pattern "create, then open" is silly, because create does
open; close internally.  Badly chosen abstraction.

>> == 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. ;-)

Fishing for a cringeworthy Friday afternoon story?

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

It's Xen, of course I'm not sure!