Re: [Qemu-devel] [PATCH 2/3] qapi: Introduce blockdev-group-snapshot-sync command

[Jeff Cody <jcody@redhat.com>]

On 02/20/2012 12:41 PM, Eric Blake wrote:
> On 02/20/2012 10:31 AM, Jeff Cody wrote:
>> This is a QAPI/QMP only command to take a snapshot of a group of
>> devices. This is simlar to the blockdev-snapshot-sync command, except
>
> s/simlar/similar/
>

Oops - fixed for v2.


>> blockdev-group-snapshot-sync accepts a list devices, filenames, and
>> formats.
>>
>> It is attempted to keep the snapshot of the group atomic; if
>> any snapshot of a device in a given group fails, then the whole group
>> is reverted back to its original image, and error is reported.
>>
>> This allows the group of disks to remain consistent with each other,
>> even across snapshot failures.
>>
>> Signed-off-by: Jeff Cody<jcody@redhat.com>
>> ---
>>   blockdev.c       |  130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qapi-schema.json |   45 +++++++++++++++++++
>>   qmp-commands.hx  |   39 ++++++++++++++++
>>   3 files changed, 214 insertions(+), 0 deletions(-)
>
>> +
>> +error_rollback:
>> +    /* failure, undo everything as much as we can */
>> +    QSIMPLEQ_FOREACH(snap_entry,&gsnp_list, entry) {
>> +        if (snap_entry->has_pivoted) {
>> +            ret = bdrv_open(snap_entry->bs, snap_entry->old_filename,
>> +                            snap_entry->flags, snap_entry->old_drv);
>> +            if (ret != 0) {
>> +                /* This is very very bad */
>> +                error_set(errp, QERR_OPEN_FILE_FAILED,
>> +                          snap_entry->old_filename);
>
> Is there any way to reduce the likelihood of a rollback failure?

Good question.  Obviously, it is ideal for this to have the lowest 
possible likelihood of a rollback failure.  I guess the real question 
is: What are the events that would cause the original image re-open to 
fail, and how can we avoid them - or, as Kevin mentioned to me, do we 
never close the original file to avoid the re-open (and, will that avoid 
those failure events)?  Hopefully these are rare events, in any case.

>
>
>> +SQMP
>> +blockdev-group-snapshot-sync
>> +----------------------
>> +
>> +Synchronous snapshot of one or more block devices.  A list array input
>> +is accepted, that contains the device, snapshot-file to be create as the
>> +target of the new image. If the file exists, or if it is a device, the
>> +snapshot will be created in the existing file/device. If does not
>> +exist, a new file will be created. format specifies the format of the
>> +snapshot image, default is qcow2.  On failure of any device, it is
>> +attempted to reopen the original image for all the devices that were
>> +specified.
>> +
>> +Arguments:
>> +
>> +- "device": device name to snapshot (json-string)
>> +- "snapshot-file": name of new image file (json-string)
>> +- "format": format of new image (json-string, optional)
>
> Shouldn't this mention that the arguments is a JSON list, rather than a
> single argument?

Yes, you are right - adding that in.

>
>> +
>> +Example:
>> +
>> +->  { "execute": "blockdev-group-snapshot-sync", "arguments": [{ "device": "ide-hd0",
>> +                                                                "snapshot-file":
>> +                                                                "/some/place/my-image",
>> +                                                                "format": "qcow2" },
>> +                                                              { "device": "ide-hd1",
>> +                                                                "snapshot-file":
>> +                                                                "/some/place/my-image2",
>> +                                                                "format": "qcow2" } }
>
> Are you missing a ']' before the final '}' here?
>

Yes, thanks.  Forgot that in the example.  Added for v2.

Jeff