From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44177) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RzpD0-0003rU-Sa for qemu-devel@nongnu.org; Tue, 21 Feb 2012 07:52:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RzpCw-0007Ph-Ls for qemu-devel@nongnu.org; Tue, 21 Feb 2012 07:52:42 -0500 Received: from mx1.redhat.com ([209.132.183.28]:30527) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RzpCw-0007Pc-Dv for qemu-devel@nongnu.org; Tue, 21 Feb 2012 07:52:38 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q1LCqb9s006912 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 21 Feb 2012 07:52:37 -0500 Message-ID: <4F439392.1060304@redhat.com> Date: Tue, 21 Feb 2012 07:52:34 -0500 From: Jeff Cody MIME-Version: 1.0 References: <82683103d1b30237a58f760ee068f98e0a9b4dd1.1329758006.git.jcody@redhat.com> <4F4285D8.3030509@redhat.com> In-Reply-To: <4F4285D8.3030509@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] qapi: Introduce blockdev-group-snapshot-sync command Reply-To: jcody@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , Luiz Capitulino , qemu-devel@nongnu.org, Markus Armbruster 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 >> --- >> 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