Re: [Qemu-devel] [PATCH 28/47] qmp: add drive-mirror command

[Paolo Bonzini <pbonzini@redhat.com>]

Il 27/07/2012 01:42, Eric Blake ha scritto:
> On 07/24/2012 05:04 AM, Paolo Bonzini wrote:
>> This adds the monitor commands that start the mirroring job.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  blockdev.c       |  133 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  hmp-commands.hx  |   21 +++++++++
>>  hmp.c            |   28 ++++++++++++
>>  hmp.h            |    1 +
>>  qapi-schema.json |   35 ++++++++++++++
>>  qmp-commands.hx  |   42 +++++++++++++++++
>>  trace-events     |    2 +-
>>  7 files changed, 258 insertions(+), 4 deletions(-)
> 
> This command only allows mirroring from the top or the complete disk,
> but no intermediate points.  That is, given:
> base <- sn1 <- sn2
> I can mirror sn2 in isolation, or the entire chain including base, but I
> cannot mirror exactly sn1+sn2.  Instead, I would have to do a mirror of
> sn2 then follow up with a partial block-stream to rebase that mirror on
> top of base.  I guess this is okay, but it feels a bit limited to have
> to do it in two steps instead of one.

I couldn't think of a use case for this:

- preparing for a failing disk requires to keep all snapshots
unmodified, so mirroring only the top

- migration with non-shared storage requires to copy the whole disk
contents (unless you want to do part of the copy outside QEMU)

- collapsing a "tower" of images to raw for performance requires a copy
of the whole disk contents

And we need the sync parameter anyway (for 'none' and in the future for
the dirty bitmap), so I preferred to keep the API simple.

> [Hmm, your later patches introduce the ability to have a persistent
> mapping file; perhaps this can be exploited to have the user pre-create
> a mapping file that shows the portions allocated by sn1+sn2 as the
> starting point, but I'm not there in the patch series yet to know if
> this is the case.]

These are not in the posted patches (and in fact have not been developed
yet :)).

>> +++ b/qapi-schema.json
>> @@ -1367,6 +1367,41 @@
>>    'returns': 'str' }
>>  
>>  ##
>> +# @drive-mirror
>> +#
>> +# Start mirroring a block device's writes to a new destination.
>> +#
>> +# @device:  the name of the device whose writes should be mirrored.
>> +#
>> +# @target: the target of the new image. If the file exists, or if it
>> +#          is a device, the existing file/device will be used as the new
>> +#          destination.  If it does not exist, a new file will be created.
>> +#
>> +# @format: #optional the format of the new destination, default is to
>> +#          probe is @mode is 'existing', else the format of the source
> 
> s/probe is/probe if/
> 
>> +#
>> +# @mode: #optional whether and how QEMU should create a new image, default is
>> +#        'absolute-paths'.
>> +#
>> +# @speed:  #optional the maximum speed, in bytes per second
>> +#
>> +# @sync: what parts of the disk image should be copied to the destination
>> +#        (all the disk, only the sectors allocated in the topmost image, or
>> +#        only new I/O).
> 
> Is there any reason 'sync' is listed last here, but before 'mode' in the
> JSON?

No, no reason.

>> +#
>> +# Returns: nothing on success
>> +#          If @device is not a valid block device, DeviceNotFound
>> +#          If @target can't be opened, OpenFileFailed
>> +#          If @format is invalid, InvalidBlockFormat
>> +#
>> +# Since 1.2
>> +##
>> +{ 'command': 'drive-mirror',
>> +  'data': { 'device': 'str', 'target': 'str', '*format': 'str',
>> +            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
>> +            '*speed': 'int' } }
>> +
> 
>> +drive-mirror
>> +------------
>> +
>> +Start mirroring a block device's writes to a new destination. target
>> +specifies the target of the new image. If the file exists, or if it is
>> +a device, it will be used as the new destination for writes. If does not
>> +exist, a new file will be created. format specifies the format of the
>> +mirror image, default is to probe if mode='existing', else the format
>> +of the source.
>> +
>> +Arguments:
>> +
>> +- "device": device name to operate on (json-string)
>> +- "target": name of new image file (json-string)
>> +- "format": format of new image (json-string, optional)
>> +- "mode": how an image file should be created into the target
>> +  file/device (NewImageMode, optional, default 'absolute-paths')
> 
> Should we call out all the possible 'NewImageMode' strings here,...
> 
>> +- "speed": maximum speed of the streaming job, in bytes per second
>> +  (json-int)
> 
> Mention that this is optional.
> 
>> +- "sync": what parts of the disk image should be copied to the destination;
>> +  possibilities include "full" for all the disk, "top" for only the sectors
>> +  allocated in the topmost image, or "none" to only replicate new I/O
>> +  (MirrorSyncMode).
> 
> ...given that we did the same for all the possible MirrorSyncMode strings?

We could.  The difference between the two is that NewImageMode is used
also in other commands.  In the end, I'm not even sure of the usefulness
of this documentation since the .json schema is much better and could be
converted to a format with hyperlinks.

Paolo