Re: [libvirt] [PATCHv2 3/3] snapshot: rudimentary qemu support for atomic disk snapshot

2012-03-19 Thread Peter Krempa 

On 03/17/2012 05:33 PM, Eric Blake wrote:

Taking an external snapshot of just one disk is atomic, without having
to pause and resume the VM.  This also paves the way for later patches
to interact with the new qemu 'transaction' monitor command.

The various scenarios when requesting atomic are:
online, 1 disk, old qemu - safe, allowed by this patch
online, more than 1 disk, old qemu - failure, this patch
offline snapshot - safe, once a future patch implements offline disk snapshot
online, 1 or more disks, new qemu - safe, once future patch uses transaction

Taking an online system checkpoint snapshot is atomic, since it is
done via a single 'savevm' monitor command.

Taking an offline system checkpoint snapshot currently uses multiple
qemu-img invocations with no provision for cleanup of partial failure,
so for now we mark it unsupported.

* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Support
new flag for single-disk setups.
(qemuDomainSnapshotDiskPrepare): Check for atomic here.
(qemuDomainSnapshotCreateDiskActive): Skip pausing the VM when
atomic supported.
(qemuDomainSnapshotIsAllowed): Use bool instead of int.
---

Patches 1/3 and 2/3 unchanged.
v2: consider interactions of atomic with non-disk-snapshots

  src/qemu/qemu_driver.c |   56 +--
  1 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 85f8cf7..9e62738 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10279,6 +10295,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
 vm, snap, flags)  0)
  goto cleanup;
  } else if (!virDomainObjIsActive(vm)) {
+if (flags  VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) {
+qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+_(atomic snapshots of inactive domains not 
+  implemented yet));
+goto cleanup;


I wonder if we shouldn't add a error code for unimplemented 
functionality to differentiate it from unsupported configurations. 
(Well, but using a configuration that uses unimplemented functionality 
makes it an unsupported configuration basically, so I don't really mind)




+}
  if (qemuDomainSnapshotCreateInactive(driver, vm, snap)  0)
  goto cleanup;
  } else {


ACK,

Peter.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 3/3] snapshot: rudimentary qemu support for atomic disk snapshot

2012-03-19 Thread Eric Blake 
On 03/19/2012 06:20 AM, Peter Krempa wrote:
 On 03/17/2012 05:33 PM, Eric Blake wrote:
 Taking an external snapshot of just one disk is atomic, without having
 to pause and resume the VM.  This also paves the way for later patches
 to interact with the new qemu 'transaction' monitor command.


 +++ b/src/qemu/qemu_driver.c
 @@ -10279,6 +10295,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
  vm, snap, flags)  0)
   goto cleanup;
   } else if (!virDomainObjIsActive(vm)) {
 +if (flags  VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) {
 +qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
 +_(atomic snapshots of inactive domains
 not 
 +  implemented yet));
 +goto cleanup;
 
 I wonder if we shouldn't add a error code for unimplemented
 functionality to differentiate it from unsupported configurations.
 (Well, but using a configuration that uses unimplemented functionality
 makes it an unsupported configuration basically, so I don't really mind)

This exact same error message is removed in 4/3, so I don't think it
quite matters _what_ we put here.  I guess I could rebase the series to
swap 3/4 and 4/4, to avoid the churn.

 
 
 +}
   if (qemuDomainSnapshotCreateInactive(driver, vm, snap)  0)
   goto cleanup;
   } else {
 
 ACK,

Thanks for the reviews.  I may wait another day or two before actually
pushing, since I'm still in the middle of testing other snapshot-related
patches, just to make sure my tests don't turn up any other needed
last-minute changes to the ones I've posted so far.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCHv2 3/3] snapshot: rudimentary qemu support for atomic disk snapshot

2012-03-17 Thread Eric Blake 
Taking an external snapshot of just one disk is atomic, without having
to pause and resume the VM.  This also paves the way for later patches
to interact with the new qemu 'transaction' monitor command.

The various scenarios when requesting atomic are:
online, 1 disk, old qemu - safe, allowed by this patch
online, more than 1 disk, old qemu - failure, this patch
offline snapshot - safe, once a future patch implements offline disk snapshot
online, 1 or more disks, new qemu - safe, once future patch uses transaction

Taking an online system checkpoint snapshot is atomic, since it is
done via a single 'savevm' monitor command.

Taking an offline system checkpoint snapshot currently uses multiple
qemu-img invocations with no provision for cleanup of partial failure,
so for now we mark it unsupported.

* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Support
new flag for single-disk setups.
(qemuDomainSnapshotDiskPrepare): Check for atomic here.
(qemuDomainSnapshotCreateDiskActive): Skip pausing the VM when
atomic supported.
(qemuDomainSnapshotIsAllowed): Use bool instead of int.
---

Patches 1/3 and 2/3 unchanged.
v2: consider interactions of atomic with non-disk-snapshots

 src/qemu/qemu_driver.c |   56 +--
 1 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 85f8cf7..9e62738 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9535,7 +9535,8 @@ cleanup:
 return ret;
 }

-static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm)
+static bool
+qemuDomainSnapshotIsAllowed(virDomainObjPtr vm)
 {
 int i;

@@ -9550,11 +9551,11 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr 
vm)
 qemuReportError(VIR_ERR_OPERATION_INVALID,
 _(Disk '%s' does not support snapshotting),
 vm-def-disks[i]-src);
-return 0;
+return false;
 }
 }

-return 1;
+return true;
 }

 static int
@@ -9712,13 +9713,17 @@ endjob:

 static int
 qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def,
-  bool allow_reuse)
+  unsigned int *flags)
 {
 int ret = -1;
 int i;
 bool found = false;
 bool active = virDomainObjIsActive(vm);
 struct stat st;
+bool allow_reuse = (*flags  VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
+bool atomic = (*flags  VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0;
+int external = 0;
+qemuDomainObjPrivatePtr priv = vm-privateData;

 for (i = 0; i  def-ndisks; i++) {
 virDomainSnapshotDiskDefPtr disk = def-disks[i];
@@ -9773,6 +9778,7 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, 
virDomainSnapshotDefPtr def,
 goto cleanup;
 }
 found = true;
+external++;
 break;

 case VIR_DOMAIN_DISK_SNAPSHOT_NO:
@@ -9792,6 +9798,17 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, 
virDomainSnapshotDefPtr def,
   selected for snapshot));
 goto cleanup;
 }
+if (active) {
+if (external == 1 ||
+qemuCapsGet(priv-qemuCaps, QEMU_CAPS_TRANSACTION)) {
+*flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC;
+} else if (atomic  external  1) {
+qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+_(atomic live snapshot of multiple disks 
+  is unsupported));
+goto cleanup;
+}
+}

 ret = 0;

@@ -9920,6 +9937,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
 int i;
 bool persist = false;
 int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */
+bool atomic = (flags  VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0;

 if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY)  0)
 return -1;
@@ -9944,14 +9962,14 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
 }
 }

-if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
-/* In qemu, snapshot_blkdev on a single disk will pause cpus,
- * but this confuses libvirt since notifications are not given
- * when qemu resumes.  And for multiple disks, libvirt must
- * pause externally to get all snapshots to be at the same
- * point in time.  For simplicitly, we always pause ourselves
- * rather than relying on qemu doing pause.
- */
+/* For multiple disks, libvirt must pause externally to get all
+ * snapshots to be at the same point in time, unless qemu supports
+ * transactions.  For a single disk, snapshot is atomic without
+ * requiring a pause.  Thanks to qemuDomainSnapshotDiskPrepare, if
+ * we got to this point, the atomic flag now says whether we need
+ * to pause, and a capability bit says whether to use transaction.
+ */
+if