Re: [Qemu-devel] blockdev operations [was: KVM call agenda for Tuesday 28th]

[Kevin Wolf]

Am 28.02.2012 17:07, schrieb Eric Blake:
> On 02/28/2012 07:58 AM, Stefan Hajnoczi wrote:
>> On Tue, Feb 28, 2012 at 2:47 PM, Paolo Bonzini <address@hidden> wrote:
>>> Il 28/02/2012 15:39, Stefan Hajnoczi ha scritto:
>>>> I'm not a fan of transactions or freeze/thaw (if used to atomically
>>>> perform other commands).
>>>>
>>>> We should not export low-level block device operations so that
>>>> external software can micromanage via QMP.  I don't think this is a
>>>> good idea because it takes the block device offline and possibly
>>>> blocks the VM.  We're reaching a level comparable to an HTTP interface
>>>> for acquiring pthread mutex, doing some operations, and then another
>>>> HTTP request to unlock it.  This is micromanagement it will create
>>>> more problems because we will have to support lots of little API
>>>> functions.
>>>
>>> So you're for extending Jeff's patches to group mirroring etc.?
>>>
>>> That's also my favorite one, assuming we can do it in time for 1.1.
>>
>> Yes, that's the approach I like the most.  It's relatively clean and
>> leaves us space to develop -blockdev.
> 
> Here's the idea I was forming based on today's call:
> 
> Jeff's idea of a group operation can be extended to allow multiple
> operations while reusing the framework.  For oVirt, we need the ability
> to open a mirror (by passing the mirror file alongside the name of the
> new external snapshot), as well as reopening a blockdev (to pivot to the
> other side of an already-open mirror).
> 
> Is there a way to express a designated union in QMP?  I'm thinking
> something along the lines of having the overall group command take a
> list of operations, where each operation can either be 'create a
> snapshot', 'create a snapshot and mirror', or 'reopen a mirror'.
> 
> I'm thinking it might look something like:
> 
> { 'enum': 'BlockdevOp',
>   'data': [ 'snapshot', 'snapshot-mirror', 'reopen' ] }
> { 'type': 'BlockdevAction',
>   'data': {'device': 'str', 'op': 'BlockdevOp',
>            'file': 'str', '*format': 'str', '*reuse': 'bool',
>            '*mirror': 'str', '*mirror-format': 'str' } }
> { 'command': 'blkdev-group-action-sync',
>   'data': { 'actionlist': [ 'BlockdevAction' ] } }

I think the general approach is good.

Your implementation in QAPI is kind of ugly because you mix the
arguments of all three commands in BlockdevAction (how about
extensibility? And the "optional" flags aren't the full truth either,
we'd have to add checks in the handlers.). We really want to have some
real union support in QAPI that creates three different C structs.

So something like (I'll reintroduce the bad word transaction, because
it's really what we're doing here, just everything in one command):

{ 'type': 'BlockdevSnapshot',
  'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
{ 'union': 'BlockdevAction',
  'types': {
       'snapshot': 'BlockdevSnapshot',
       'mirror': 'BlockdevMirror', ...
   } }
{ 'command': 'blockdev-transaction',
  'data': { 'actionlist': [ 'BlockdevAction' ] } }

On the wire in QMP this would look like:

{ 'execute': 'blockdev-transaction',
  'arguments': {
    'actionlist': [
      { 'type': 'snapshot',
        'data': { 'device': 'ide-hd0', ... } },
      { 'type': 'mirror',
        'data': { ... }
    ]
  }
}

Now if you look closely, BlockdevSnapshot is exactly what Jeff called
SnapshotDev in his series, which in turn is the definition of the
blockdev-snapshot-sync command. We can reuse all of this even in the API
of a new more generic command.

So my conclusion for now is that we can merge the group snapshots right
now instead of waiting for the mirror design to be completed, and we can
reuse everything in a more complex transaction command later.

Kevin