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

[Jeff Cody]

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<address@hidden>
> > ---
> >   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