All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes
@ 2015-11-07 15:54 Denis V. Lunev
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 01/10] snapshot: create helper to test that block drivers supports snapshots Denis V. Lunev
                   ` (10 more replies)
  0 siblings, 11 replies; 18+ messages in thread
From: Denis V. Lunev @ 2015-11-07 15:54 UTC (permalink / raw)
  Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, Juan Quintela

with test
    while /bin/true ; do
        virsh snapshot-create rhel7
        sleep 10
        virsh snapshot-delete rhel7 --current
    done
with enabled iothreads on a running VM leads to a lot of troubles: hangs,
asserts, errors.

Anyway, I think that the construction like
    assert(aio_context_is_locked(aio_context));
should be widely used to ensure proper locking.

Changes from v5:
- dropped already merged patch 11
- fixed spelling in patch 1
- changed order of condition in loops in all patches. Thank you Stefan :)
- dropped patch 9
- aio_context is not acquired any more in bdrv_all_find_vmstate_bs by request
  of Stefan
- patch 10 is implemented in completely different way

Changes from v4:
- only migration/savevm.c code and monitor is affected now. Generic block
  layer stuff will be sent separately to speedup merging. The approach
  in general was negotiated with Juan and Stefan.

Changes from v3:
- more places found
- new aio_poll concept, see patch 10

Changes from v2:
- droppped patch 5 as already merged
- changed locking scheme in patch 4 by suggestion of Juan

Changes from v1:
- aio-context locking added
- comment is rewritten

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>

Denis V. Lunev (10):
  snapshot: create helper to test that block drivers supports snapshots
  snapshot: return error code from bdrv_snapshot_delete_by_id_or_name
  snapshot: create bdrv_all_delete_snapshot helper
  snapshot: create bdrv_all_goto_snapshot helper
  snapshot: create bdrv_all_find_snapshot helper
  migration: drop find_vmstate_bs check in hmp_delvm
  snapshot: create bdrv_all_create_snapshot helper
  migration: reorder processing in hmp_savevm
  migration: implement bdrv_all_find_vmstate_bs helper
  migration: normalize locking in migration/savevm.c

 block/snapshot.c         | 130 ++++++++++++++++++++++++++++-
 include/block/snapshot.h |  21 ++++-
 migration/savevm.c       | 210 +++++++++++++++--------------------------------
 3 files changed, 209 insertions(+), 152 deletions(-)

-- 
2.5.0

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH 01/10] snapshot: create helper to test that block drivers supports snapshots
  2015-11-07 15:54 [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Denis V. Lunev
@ 2015-11-07 15:54 ` Denis V. Lunev
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 02/10] snapshot: return error code from bdrv_snapshot_delete_by_id_or_name Denis V. Lunev
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Denis V. Lunev @ 2015-11-07 15:54 UTC (permalink / raw)
  Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, Juan Quintela

The patch enforces proper locking for this operation.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/snapshot.c         | 24 ++++++++++++++++++++++++
 include/block/snapshot.h |  8 ++++++++
 migration/savevm.c       | 17 ++++-------------
 3 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 89500f2..d929d08 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -356,3 +356,27 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
 
     return ret;
 }
+
+
+/* Group operations. All block drivers are involved.
+ * These functions will properly handle dataplane (take aio_context_acquire
+ * when appropriate for appropriate block drivers) */
+
+bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
+{
+    bool ok = true;
+    BlockDriverState *bs = NULL;
+
+    while (ok && (bs = bdrv_next(bs))) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        if (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs)) {
+            ok = bdrv_can_snapshot(bs);
+        }
+        aio_context_release(ctx);
+    }
+
+    *first_bad_bs = bs;
+    return ok;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 770d9bb..6195c9c 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -75,4 +75,12 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
 int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
                                          const char *id_or_name,
                                          Error **errp);
+
+
+/* Group operations. All block drivers are involved.
+ * These functions will properly handle dataplane (take aio_context_acquire
+ * when appropriate for appropriate block drivers */
+
+bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
+
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 9f2230f..c212288 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1290,19 +1290,10 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     const char *name = qdict_get_try_str(qdict, "name");
     Error *local_err = NULL;
 
-    /* Verify if there is a device that doesn't support snapshots and is writable */
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-
-        if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
-            continue;
-        }
-
-        if (!bdrv_can_snapshot(bs)) {
-            monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n",
-                               bdrv_get_device_name(bs));
-            return;
-        }
+    if (!bdrv_all_can_snapshot(&bs)) {
+        monitor_printf(mon, "Device '%s' is writable but does not "
+                       "support snapshots.\n", bdrv_get_device_name(bs));
+        return;
     }
 
     bs = find_vmstate_bs();
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH 02/10] snapshot: return error code from bdrv_snapshot_delete_by_id_or_name
  2015-11-07 15:54 [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Denis V. Lunev
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 01/10] snapshot: create helper to test that block drivers supports snapshots Denis V. Lunev
@ 2015-11-07 15:54 ` Denis V. Lunev
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 03/10] snapshot: create bdrv_all_delete_snapshot helper Denis V. Lunev
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Denis V. Lunev @ 2015-11-07 15:54 UTC (permalink / raw)
  Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, Juan Quintela

this will make code better in the next patch

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/snapshot.c         | 7 ++++---
 include/block/snapshot.h | 6 +++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index d929d08..ed0422d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -253,9 +253,9 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
-void bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
-                                        const char *id_or_name,
-                                        Error **errp)
+int bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
+                                       const char *id_or_name,
+                                       Error **errp)
 {
     int ret;
     Error *local_err = NULL;
@@ -270,6 +270,7 @@ void bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
     if (ret < 0) {
         error_propagate(errp, local_err);
     }
+    return ret;
 }
 
 int bdrv_snapshot_list(BlockDriverState *bs,
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 6195c9c..9ddfd42 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -63,9 +63,9 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
                          const char *snapshot_id,
                          const char *name,
                          Error **errp);
-void bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
-                                        const char *id_or_name,
-                                        Error **errp);
+int bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
+                                       const char *id_or_name,
+                                       Error **errp);
 int bdrv_snapshot_list(BlockDriverState *bs,
                        QEMUSnapshotInfo **psn_info);
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH 03/10] snapshot: create bdrv_all_delete_snapshot helper
  2015-11-07 15:54 [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Denis V. Lunev
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 01/10] snapshot: create helper to test that block drivers supports snapshots Denis V. Lunev
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 02/10] snapshot: return error code from bdrv_snapshot_delete_by_id_or_name Denis V. Lunev
@ 2015-11-07 15:54 ` Denis V. Lunev
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 04/10] snapshot: create bdrv_all_goto_snapshot helper Denis V. Lunev
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Denis V. Lunev @ 2015-11-07 15:54 UTC (permalink / raw)
  Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, Juan Quintela

to delete snapshots from all loaded block drivers.

The patch also ensures proper locking.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/snapshot.c         | 22 ++++++++++++++++++++
 include/block/snapshot.h |  2 ++
 migration/savevm.c       | 54 +++++++++---------------------------------------
 3 files changed, 34 insertions(+), 44 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index ed0422d..61a6ad1 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -381,3 +381,25 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
     *first_bad_bs = bs;
     return ok;
 }
+
+int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
+                             Error **err)
+{
+    int ret = 0;
+    BlockDriverState *bs = NULL;
+    QEMUSnapshotInfo sn1, *snapshot = &sn1;
+
+    while (ret == 0 && (bs = bdrv_next(bs))) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        if (bdrv_can_snapshot(bs) &&
+                bdrv_snapshot_find(bs, snapshot, name) >= 0) {
+            ret = bdrv_snapshot_delete_by_id_or_name(bs, name, err);
+        }
+        aio_context_release(ctx);
+    }
+
+    *first_bad_bs = bs;
+    return ret;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 9ddfd42..d02d2b1 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -82,5 +82,7 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
  * when appropriate for appropriate block drivers */
 
 bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
+int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
+                             Error **err);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index c212288..1157a6f 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1248,35 +1248,6 @@ static BlockDriverState *find_vmstate_bs(void)
     return NULL;
 }
 
-/*
- * Deletes snapshots of a given name in all opened images.
- */
-static int del_existing_snapshots(Monitor *mon, const char *name)
-{
-    BlockDriverState *bs;
-    QEMUSnapshotInfo sn1, *snapshot = &sn1;
-    Error *err = NULL;
-
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-        if (bdrv_can_snapshot(bs) &&
-            bdrv_snapshot_find(bs, snapshot, name) >= 0) {
-            bdrv_snapshot_delete_by_id_or_name(bs, name, &err);
-            if (err) {
-                monitor_printf(mon,
-                               "Error while deleting snapshot on device '%s':"
-                               " %s\n",
-                               bdrv_get_device_name(bs),
-                               error_get_pretty(err));
-                error_free(err);
-                return -1;
-            }
-        }
-    }
-
-    return 0;
-}
-
 void hmp_savevm(Monitor *mon, const QDict *qdict)
 {
     BlockDriverState *bs, *bs1;
@@ -1334,7 +1305,11 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     }
 
     /* Delete old snapshots of the same name */
-    if (name && del_existing_snapshots(mon, name) < 0) {
+    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
+        monitor_printf(mon,
+                       "Error while deleting snapshot on device '%s': %s\n",
+                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
+        error_free(local_err);
         goto the_end;
     }
 
@@ -1494,20 +1469,11 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-        if (bdrv_can_snapshot(bs)) {
-            err = NULL;
-            bdrv_snapshot_delete_by_id_or_name(bs, name, &err);
-            if (err) {
-                monitor_printf(mon,
-                               "Error while deleting snapshot on device '%s':"
-                               " %s\n",
-                               bdrv_get_device_name(bs),
-                               error_get_pretty(err));
-                error_free(err);
-            }
-        }
+    if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
+        monitor_printf(mon,
+                       "Error while deleting snapshot on device '%s': %s\n",
+                       bdrv_get_device_name(bs), error_get_pretty(err));
+        error_free(err);
     }
 }
 
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH 04/10] snapshot: create bdrv_all_goto_snapshot helper
  2015-11-07 15:54 [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Denis V. Lunev
                   ` (2 preceding siblings ...)
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 03/10] snapshot: create bdrv_all_delete_snapshot helper Denis V. Lunev
@ 2015-11-07 15:54 ` Denis V. Lunev
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper Denis V. Lunev
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Denis V. Lunev @ 2015-11-07 15:54 UTC (permalink / raw)
  Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, Juan Quintela

to switch to snapshot on all loaded block drivers.

The patch also ensures proper locking.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/snapshot.c         | 20 ++++++++++++++++++++
 include/block/snapshot.h |  1 +
 migration/savevm.c       | 15 +++++----------
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 61a6ad1..9f07a63 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -403,3 +403,23 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
     *first_bad_bs = bs;
     return ret;
 }
+
+
+int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
+{
+    int err = 0;
+    BlockDriverState *bs = NULL;
+
+    while (err == 0 && (bs = bdrv_next(bs))) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        if (bdrv_can_snapshot(bs)) {
+            err = bdrv_snapshot_goto(bs, name);
+        }
+        aio_context_release(ctx);
+    }
+
+    *first_bad_bs = bs;
+    return err;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index d02d2b1..0a176c7 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -84,5 +84,6 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
 bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
 int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
                              Error **err);
+int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 1157a6f..d18ff13 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1425,16 +1425,11 @@ int load_vmstate(const char *name)
     /* Flush all IO requests so they don't interfere with the new state.  */
     bdrv_drain_all();
 
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-        if (bdrv_can_snapshot(bs)) {
-            ret = bdrv_snapshot_goto(bs, name);
-            if (ret < 0) {
-                error_report("Error %d while activating snapshot '%s' on '%s'",
-                             ret, name, bdrv_get_device_name(bs));
-                return ret;
-            }
-        }
+    ret = bdrv_all_goto_snapshot(name, &bs);
+    if (ret < 0) {
+        error_report("Error %d while activating snapshot '%s' on '%s'",
+                     ret, name, bdrv_get_device_name(bs));
+        return ret;
     }
 
     /* restore the VM state */
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper
  2015-11-07 15:54 [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Denis V. Lunev
                   ` (3 preceding siblings ...)
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 04/10] snapshot: create bdrv_all_goto_snapshot helper Denis V. Lunev
@ 2015-11-07 15:54 ` Denis V. Lunev
  2015-11-09 17:27   ` Stefan Hajnoczi
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 06/10] migration: drop find_vmstate_bs check in hmp_delvm Denis V. Lunev
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 18+ messages in thread
From: Denis V. Lunev @ 2015-11-07 15:54 UTC (permalink / raw)
  Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, Juan Quintela

to check that snapshot is available for all loaded block drivers. The
ability to switch to snapshot is verified separately using
bdrv_all_can_snapshot.

The patch also ensures proper locking.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/snapshot.c         | 20 ++++++++++++++++++
 include/block/snapshot.h |  1 +
 migration/savevm.c       | 55 +++++++++++++-----------------------------------
 3 files changed, 36 insertions(+), 40 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 9f07a63..a7f1f8d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -423,3 +423,23 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
     *first_bad_bs = bs;
     return err;
 }
+
+int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
+{
+    QEMUSnapshotInfo sn;
+    int err = 0;
+    BlockDriverState *bs = NULL;
+
+    while (err == 0 && (bs = bdrv_next(bs))) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        if (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs)) {
+            err = bdrv_snapshot_find(bs, &sn, name);
+        }
+        aio_context_release(ctx);
+    }
+
+    *first_bad_bs = bs;
+    return err;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 0a176c7..10ee582 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -85,5 +85,6 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
 int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
                              Error **err);
 int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
+int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index d18ff13..a9c1130 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1383,6 +1383,18 @@ int load_vmstate(const char *name)
     QEMUFile *f;
     int ret;
 
+    if (!bdrv_all_can_snapshot(&bs)) {
+        error_report("Device '%s' is writable but does not support snapshots.",
+                     bdrv_get_device_name(bs));
+        return -ENOTSUP;
+    }
+    ret = bdrv_all_find_snapshot(name, &bs);
+    if (ret < 0) {
+        error_report("Device '%s' does not have the requested snapshot '%s'",
+                     bdrv_get_device_name(bs), name);
+        return ret;
+    }
+
     bs_vm_state = find_vmstate_bs();
     if (!bs_vm_state) {
         error_report("No block device supports snapshots");
@@ -1399,29 +1411,6 @@ int load_vmstate(const char *name)
         return -EINVAL;
     }
 
-    /* Verify if there is any device that doesn't support snapshots and is
-    writable and check if the requested snapshot is available too. */
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-
-        if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
-            continue;
-        }
-
-        if (!bdrv_can_snapshot(bs)) {
-            error_report("Device '%s' is writable but does not support snapshots.",
-                               bdrv_get_device_name(bs));
-            return -ENOTSUP;
-        }
-
-        ret = bdrv_snapshot_find(bs, &sn, name);
-        if (ret < 0) {
-            error_report("Device '%s' does not have the requested snapshot '%s'",
-                           bdrv_get_device_name(bs), name);
-            return ret;
-        }
-    }
-
     /* Flush all IO requests so they don't interfere with the new state.  */
     bdrv_drain_all();
 
@@ -1475,8 +1464,8 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 {
     BlockDriverState *bs, *bs1;
-    QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
-    int nb_sns, i, ret, available;
+    QEMUSnapshotInfo *sn_tab, *sn;
+    int nb_sns, i;
     int total;
     int *available_snapshots;
 
@@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     available_snapshots = g_new0(int, nb_sns);
     total = 0;
     for (i = 0; i < nb_sns; i++) {
-        sn = &sn_tab[i];
-        available = 1;
-        bs1 = NULL;
-
-        while ((bs1 = bdrv_next(bs1))) {
-            if (bdrv_can_snapshot(bs1) && bs1 != bs) {
-                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
-                if (ret < 0) {
-                    available = 0;
-                    break;
-                }
-            }
-        }
-
-        if (available) {
+        if (bdrv_all_find_snapshot(sn_tab[i].id_str, &bs1) == 0) {
             available_snapshots[total] = i;
             total++;
         }
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH 06/10] migration: drop find_vmstate_bs check in hmp_delvm
  2015-11-07 15:54 [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Denis V. Lunev
                   ` (4 preceding siblings ...)
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper Denis V. Lunev
@ 2015-11-07 15:54 ` Denis V. Lunev
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 07/10] snapshot: create bdrv_all_create_snapshot helper Denis V. Lunev
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Denis V. Lunev @ 2015-11-07 15:54 UTC (permalink / raw)
  Cc: Denis V. Lunev, qemu-devel, Juan Quintela

There is no much sense to do the check and write warning.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
---
 migration/savevm.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index a9c1130..0c2890d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1448,11 +1448,6 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
     Error *err;
     const char *name = qdict_get_str(qdict, "name");
 
-    if (!find_vmstate_bs()) {
-        monitor_printf(mon, "No block device supports snapshots\n");
-        return;
-    }
-
     if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
         monitor_printf(mon,
                        "Error while deleting snapshot on device '%s': %s\n",
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH 07/10] snapshot: create bdrv_all_create_snapshot helper
  2015-11-07 15:54 [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Denis V. Lunev
                   ` (5 preceding siblings ...)
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 06/10] migration: drop find_vmstate_bs check in hmp_delvm Denis V. Lunev
@ 2015-11-07 15:54 ` Denis V. Lunev
  2015-11-09 17:33   ` Stefan Hajnoczi
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 08/10] migration: reorder processing in hmp_savevm Denis V. Lunev
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 18+ messages in thread
From: Denis V. Lunev @ 2015-11-07 15:54 UTC (permalink / raw)
  Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, Juan Quintela

to create snapshot for all loaded block drivers.

The patch also ensures proper locking.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/snapshot.c         | 22 ++++++++++++++++++++++
 include/block/snapshot.h |  1 +
 migration/savevm.c       | 20 +++++---------------
 3 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index a7f1f8d..4daf9d4 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -443,3 +443,25 @@ int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
     *first_bad_bs = bs;
     return err;
 }
+
+int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn, BlockDriverState **bad)
+{
+    int err = 0;
+    BlockDriverState *bs = NULL;
+
+    while (err == 0 && (bs = bdrv_next(bs))) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        if (bdrv_can_snapshot(bs)) {
+            err = bdrv_snapshot_create(bs, sn);
+            /* Tricky part here. First image contains VM state. The behavior
+             * is matched one in bdrv_all_find_vmstate_bs */
+            sn->vm_state_size = 0;
+        }
+        aio_context_release(ctx);
+    }
+
+    *bad = bs;
+    return err;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 10ee582..df443b2 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -86,5 +86,6 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
                              Error **err);
 int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
 int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
+int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn, BlockDriverState **bad);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 0c2890d..f72349d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1255,7 +1255,6 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     int ret;
     QEMUFile *f;
     int saved_vm_running;
-    uint64_t vm_state_size;
     qemu_timeval tv;
     struct tm tm;
     const char *name = qdict_get_try_str(qdict, "name");
@@ -1320,7 +1319,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         goto the_end;
     }
     ret = qemu_savevm_state(f, &local_err);
-    vm_state_size = qemu_ftell(f);
+    sn->vm_state_size = qemu_ftell(f);
     qemu_fclose(f);
     if (ret < 0) {
         monitor_printf(mon, "%s\n", error_get_pretty(local_err));
@@ -1328,19 +1327,10 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         goto the_end;
     }
 
-    /* create the snapshots */
-
-    bs1 = NULL;
-    while ((bs1 = bdrv_next(bs1))) {
-        if (bdrv_can_snapshot(bs1)) {
-            /* Write VM state size only to the image that contains the state */
-            sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
-            ret = bdrv_snapshot_create(bs1, sn);
-            if (ret < 0) {
-                monitor_printf(mon, "Error while creating snapshot on '%s'\n",
-                               bdrv_get_device_name(bs1));
-            }
-        }
+    ret = bdrv_all_create_snapshot(sn, &bs);
+    if (ret < 0) {
+        monitor_printf(mon, "Error while creating snapshot on '%s'\n",
+                       bdrv_get_device_name(bs));
     }
 
  the_end:
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH 08/10] migration: reorder processing in hmp_savevm
  2015-11-07 15:54 [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Denis V. Lunev
                   ` (6 preceding siblings ...)
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 07/10] snapshot: create bdrv_all_create_snapshot helper Denis V. Lunev
@ 2015-11-07 15:54 ` Denis V. Lunev
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 09/10] migration: implement bdrv_all_find_vmstate_bs helper Denis V. Lunev
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 18+ messages in thread
From: Denis V. Lunev @ 2015-11-07 15:54 UTC (permalink / raw)
  Cc: Denis V. Lunev, qemu-devel, Juan Quintela

State deletion can be performed on running VM which reduces VM downtime
This approach looks a bit more natural.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
---
 migration/savevm.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index f72349d..b4a3930 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1266,6 +1266,15 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         return;
     }
 
+    /* Delete old snapshots of the same name */
+    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
+        monitor_printf(mon,
+                       "Error while deleting snapshot on device '%s': %s\n",
+                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
+        error_free(local_err);
+        return;
+    }
+
     bs = find_vmstate_bs();
     if (!bs) {
         monitor_printf(mon, "No block device can accept snapshots\n");
@@ -1303,15 +1312,6 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
     }
 
-    /* Delete old snapshots of the same name */
-    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
-        monitor_printf(mon,
-                       "Error while deleting snapshot on device '%s': %s\n",
-                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
-        error_free(local_err);
-        goto the_end;
-    }
-
     /* save the VM state */
     f = qemu_fopen_bdrv(bs, 1);
     if (!f) {
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH 09/10] migration: implement bdrv_all_find_vmstate_bs helper
  2015-11-07 15:54 [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Denis V. Lunev
                   ` (7 preceding siblings ...)
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 08/10] migration: reorder processing in hmp_savevm Denis V. Lunev
@ 2015-11-07 15:54 ` Denis V. Lunev
  2015-11-07 15:55 ` [Qemu-devel] [PATCH 10/10] migration: normalize locking in migration/savevm.c Denis V. Lunev
  2015-11-09 17:37 ` [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Stefan Hajnoczi
  10 siblings, 0 replies; 18+ messages in thread
From: Denis V. Lunev @ 2015-11-07 15:54 UTC (permalink / raw)
  Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, Juan Quintela

The patch also ensures proper locking for the operation.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/snapshot.c         | 15 +++++++++++++++
 include/block/snapshot.h |  2 ++
 migration/savevm.c       | 19 ++++---------------
 3 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 4daf9d4..337ac8e 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -465,3 +465,18 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn, BlockDriverState **bad)
     *bad = bs;
     return err;
 }
+
+BlockDriverState *bdrv_all_find_vmstate_bs(void)
+{
+    bool not_found = true;
+    BlockDriverState *bs = NULL;
+
+    while (not_found && (bs = bdrv_next(bs))) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        not_found = !bdrv_can_snapshot(bs);
+        aio_context_release(ctx);
+    }
+    return bs;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index df443b2..543cbb3 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -88,4 +88,6 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
 int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
 int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn, BlockDriverState **bad);
 
+BlockDriverState *bdrv_all_find_vmstate_bs(void);
+
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index b4a3930..6be30cb 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1237,17 +1237,6 @@ out:
     return ret;
 }
 
-static BlockDriverState *find_vmstate_bs(void)
-{
-    BlockDriverState *bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-        if (bdrv_can_snapshot(bs)) {
-            return bs;
-        }
-    }
-    return NULL;
-}
-
 void hmp_savevm(Monitor *mon, const QDict *qdict)
 {
     BlockDriverState *bs, *bs1;
@@ -1275,8 +1264,8 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    bs = find_vmstate_bs();
-    if (!bs) {
+    bs = bdrv_all_find_vmstate_bs();
+    if (bs == NULL) {
         monitor_printf(mon, "No block device can accept snapshots\n");
         return;
     }
@@ -1385,7 +1374,7 @@ int load_vmstate(const char *name)
         return ret;
     }
 
-    bs_vm_state = find_vmstate_bs();
+    bs_vm_state = bdrv_all_find_vmstate_bs();
     if (!bs_vm_state) {
         error_report("No block device supports snapshots");
         return -ENOTSUP;
@@ -1454,7 +1443,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     int total;
     int *available_snapshots;
 
-    bs = find_vmstate_bs();
+    bs = bdrv_all_find_vmstate_bs();
     if (!bs) {
         monitor_printf(mon, "No available block device supports snapshots\n");
         return;
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH 10/10] migration: normalize locking in migration/savevm.c
  2015-11-07 15:54 [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Denis V. Lunev
                   ` (8 preceding siblings ...)
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 09/10] migration: implement bdrv_all_find_vmstate_bs helper Denis V. Lunev
@ 2015-11-07 15:55 ` Denis V. Lunev
  2015-11-09 17:37 ` [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Stefan Hajnoczi
  10 siblings, 0 replies; 18+ messages in thread
From: Denis V. Lunev @ 2015-11-07 15:55 UTC (permalink / raw)
  Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, Juan Quintela

basically all bdrv_* operations must be called under aio_context_acquire
except ones with bdrv_all prefix.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 migration/savevm.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 6be30cb..5e6ff55 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1248,6 +1248,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     struct tm tm;
     const char *name = qdict_get_try_str(qdict, "name");
     Error *local_err = NULL;
+    AioContext *aio_context;
 
     if (!bdrv_all_can_snapshot(&bs)) {
         monitor_printf(mon, "Device '%s' is writable but does not "
@@ -1269,6 +1270,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "No block device can accept snapshots\n");
         return;
     }
+    aio_context = bdrv_get_aio_context(bs);
 
     saved_vm_running = runstate_is_running();
 
@@ -1279,6 +1281,8 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     }
     vm_stop(RUN_STATE_SAVE_VM);
 
+    aio_context_acquire(aio_context);
+
     memset(sn, 0, sizeof(*sn));
 
     /* fill auxiliary fields */
@@ -1323,6 +1327,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     }
 
  the_end:
+    aio_context_release(aio_context);
     if (saved_vm_running) {
         vm_start();
     }
@@ -1361,6 +1366,7 @@ int load_vmstate(const char *name)
     QEMUSnapshotInfo sn;
     QEMUFile *f;
     int ret;
+    AioContext *aio_context;
 
     if (!bdrv_all_can_snapshot(&bs)) {
         error_report("Device '%s' is writable but does not support snapshots.",
@@ -1379,9 +1385,12 @@ int load_vmstate(const char *name)
         error_report("No block device supports snapshots");
         return -ENOTSUP;
     }
+    aio_context = bdrv_get_aio_context(bs);
 
     /* Don't even try to load empty VM states */
+    aio_context_acquire(aio_context);
     ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
+    aio_context_release(aio_context);
     if (ret < 0) {
         return ret;
     } else if (sn.vm_state_size == 0) {
@@ -1409,9 +1418,12 @@ int load_vmstate(const char *name)
 
     qemu_system_reset(VMRESET_SILENT);
     migration_incoming_state_new(f);
-    ret = qemu_loadvm_state(f);
 
+    aio_context_acquire(aio_context);
+    ret = qemu_loadvm_state(f);
     qemu_fclose(f);
+    aio_context_release(aio_context);
+
     migration_incoming_state_destroy();
     if (ret < 0) {
         error_report("Error %d while loading VM state", ret);
@@ -1442,14 +1454,19 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     int nb_sns, i;
     int total;
     int *available_snapshots;
+    AioContext *aio_context;
 
     bs = bdrv_all_find_vmstate_bs();
     if (!bs) {
         monitor_printf(mon, "No available block device supports snapshots\n");
         return;
     }
+    aio_context = bdrv_get_aio_context(bs);
 
+    aio_context_acquire(aio_context);
     nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    aio_context_release(aio_context);
+
     if (nb_sns < 0) {
         monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
         return;
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper Denis V. Lunev
@ 2015-11-09 17:27   ` Stefan Hajnoczi
  0 siblings, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2015-11-09 17:27 UTC (permalink / raw)
  To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, Juan Quintela

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]

On Sat, Nov 07, 2015 at 06:54:55PM +0300, Denis V. Lunev wrote:

This:

> +int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
> +{
> +    QEMUSnapshotInfo sn;
> +    int err = 0;
> +    BlockDriverState *bs = NULL;
> +
> +    while (err == 0 && (bs = bdrv_next(bs))) {
> +        AioContext *ctx = bdrv_get_aio_context(bs);
> +
> +        aio_context_acquire(ctx);
> +        if (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs)) {
> +            err = bdrv_snapshot_find(bs, &sn, name);
> +        }
> +        aio_context_release(ctx);
> +    }
> +
> +    *first_bad_bs = bs;
> +    return err;
> +}

and this:

> @@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
>      available_snapshots = g_new0(int, nb_sns);
>      total = 0;
>      for (i = 0; i < nb_sns; i++) {
> -        sn = &sn_tab[i];
> -        available = 1;
> -        bs1 = NULL;
> -
> -        while ((bs1 = bdrv_next(bs1))) {
> -            if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> -                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
> -                if (ret < 0) {
> -                    available = 0;
> -                    break;
> -                }
> -            }
> -        }
> -
> -        if (available) {
> +        if (bdrv_all_find_snapshot(sn_tab[i].id_str, &bs1) == 0) {
>              available_snapshots[total] = i;
>              total++;
>          }

is not equivalent.  hmp_info_snapshots() skips devices that are inserted
and writeable but do not support snapshots.  The new code will fail in
that case.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH 07/10] snapshot: create bdrv_all_create_snapshot helper
  2015-11-07 15:54 ` [Qemu-devel] [PATCH 07/10] snapshot: create bdrv_all_create_snapshot helper Denis V. Lunev
@ 2015-11-09 17:33   ` Stefan Hajnoczi
  0 siblings, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2015-11-09 17:33 UTC (permalink / raw)
  To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, Juan Quintela

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]

On Sat, Nov 07, 2015 at 06:54:57PM +0300, Denis V. Lunev wrote:
> +int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn, BlockDriverState **bad)
> +{
> +    int err = 0;
> +    BlockDriverState *bs = NULL;
> +
> +    while (err == 0 && (bs = bdrv_next(bs))) {
> +        AioContext *ctx = bdrv_get_aio_context(bs);
> +
> +        aio_context_acquire(ctx);
> +        if (bdrv_can_snapshot(bs)) {
> +            err = bdrv_snapshot_create(bs, sn);
> +            /* Tricky part here. First image contains VM state. The behavior
> +             * is matched one in bdrv_all_find_vmstate_bs */
> +            sn->vm_state_size = 0;

Please avoid the tricky part by passing in vm_state_bs and
vm_state_size.  Then this function can do:

  /* Write VM state size only to the image that contains the state */
  sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);

without making assumptions about the algorithm for choosing the device
to store vmstate data on.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes
  2015-11-07 15:54 [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Denis V. Lunev
                   ` (9 preceding siblings ...)
  2015-11-07 15:55 ` [Qemu-devel] [PATCH 10/10] migration: normalize locking in migration/savevm.c Denis V. Lunev
@ 2015-11-09 17:37 ` Stefan Hajnoczi
  2015-11-09 17:57   ` Denis V. Lunev
  10 siblings, 1 reply; 18+ messages in thread
From: Stefan Hajnoczi @ 2015-11-09 17:37 UTC (permalink / raw)
  To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, Juan Quintela

[-- Attachment #1: Type: text/plain, Size: 884 bytes --]

On Sat, Nov 07, 2015 at 06:54:50PM +0300, Denis V. Lunev wrote:
> with test
>     while /bin/true ; do
>         virsh snapshot-create rhel7
>         sleep 10
>         virsh snapshot-delete rhel7 --current
>     done
> with enabled iothreads on a running VM leads to a lot of troubles: hangs,
> asserts, errors.
> 
> Anyway, I think that the construction like
>     assert(aio_context_is_locked(aio_context));
> should be widely used to ensure proper locking.
> 
> Changes from v5:
> - dropped already merged patch 11
> - fixed spelling in patch 1
> - changed order of condition in loops in all patches. Thank you Stefan :)
> - dropped patch 9
> - aio_context is not acquired any more in bdrv_all_find_vmstate_bs by request
>   of Stefan
> - patch 10 is implemented in completely different way

I left comments on specific patches.  Besides that, I'm happy.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes
  2015-11-09 17:37 ` [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Stefan Hajnoczi
@ 2015-11-09 17:57   ` Denis V. Lunev
  2015-11-10  9:45     ` Stefan Hajnoczi
  0 siblings, 1 reply; 18+ messages in thread
From: Denis V. Lunev @ 2015-11-09 17:57 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Juan Quintela

On 11/09/2015 08:37 PM, Stefan Hajnoczi wrote:
> On Sat, Nov 07, 2015 at 06:54:50PM +0300, Denis V. Lunev wrote:
>> with test
>>      while /bin/true ; do
>>          virsh snapshot-create rhel7
>>          sleep 10
>>          virsh snapshot-delete rhel7 --current
>>      done
>> with enabled iothreads on a running VM leads to a lot of troubles: hangs,
>> asserts, errors.
>>
>> Anyway, I think that the construction like
>>      assert(aio_context_is_locked(aio_context));
>> should be widely used to ensure proper locking.
>>
>> Changes from v5:
>> - dropped already merged patch 11
>> - fixed spelling in patch 1
>> - changed order of condition in loops in all patches. Thank you Stefan :)
>> - dropped patch 9
>> - aio_context is not acquired any more in bdrv_all_find_vmstate_bs by request
>>    of Stefan
>> - patch 10 is implemented in completely different way
> I left comments on specific patches.  Besides that, I'm happy.
OK. that sounds good enough to me. These changes
are not a problem at all.

Should we ask Juan that this is good for him?

Den

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes
  2015-11-09 17:57   ` Denis V. Lunev
@ 2015-11-10  9:45     ` Stefan Hajnoczi
  0 siblings, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2015-11-10  9:45 UTC (permalink / raw)
  To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, Juan Quintela

[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]

On Mon, Nov 09, 2015 at 08:57:26PM +0300, Denis V. Lunev wrote:
> On 11/09/2015 08:37 PM, Stefan Hajnoczi wrote:
> >On Sat, Nov 07, 2015 at 06:54:50PM +0300, Denis V. Lunev wrote:
> >>with test
> >>     while /bin/true ; do
> >>         virsh snapshot-create rhel7
> >>         sleep 10
> >>         virsh snapshot-delete rhel7 --current
> >>     done
> >>with enabled iothreads on a running VM leads to a lot of troubles: hangs,
> >>asserts, errors.
> >>
> >>Anyway, I think that the construction like
> >>     assert(aio_context_is_locked(aio_context));
> >>should be widely used to ensure proper locking.
> >>
> >>Changes from v5:
> >>- dropped already merged patch 11
> >>- fixed spelling in patch 1
> >>- changed order of condition in loops in all patches. Thank you Stefan :)
> >>- dropped patch 9
> >>- aio_context is not acquired any more in bdrv_all_find_vmstate_bs by request
> >>   of Stefan
> >>- patch 10 is implemented in completely different way
> >I left comments on specific patches.  Besides that, I'm happy.
> OK. that sounds good enough to me. These changes
> are not a problem at all.
> 
> Should we ask Juan that this is good for him?

I think the requested changes won't be controversial.  Sending the next
revision should be fine.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH 08/10] migration: reorder processing in hmp_savevm
  2015-11-16 15:24 [Qemu-devel] [PATCH for 2.5 v8 0/10] dataplane snapshot fixes Denis V. Lunev
@ 2015-11-16 15:24 ` Denis V. Lunev
  0 siblings, 0 replies; 18+ messages in thread
From: Denis V. Lunev @ 2015-11-16 15:24 UTC (permalink / raw)
  Cc: Denis V. Lunev, qemu-devel, stefanha, Juan Quintela

State deletion can be performed on running VM which reduces VM downtime
This approach looks a bit more natural.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Fam Zheng <famz@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
---
 migration/savevm.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index b004f84..9ec9117 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1935,6 +1935,15 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         return;
     }
 
+    /* Delete old snapshots of the same name */
+    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
+        monitor_printf(mon,
+                       "Error while deleting snapshot on device '%s': %s\n",
+                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
+        error_free(local_err);
+        return;
+    }
+
     bs = find_vmstate_bs();
     if (!bs) {
         monitor_printf(mon, "No block device can accept snapshots\n");
@@ -1972,15 +1981,6 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
     }
 
-    /* Delete old snapshots of the same name */
-    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
-        monitor_printf(mon,
-                       "Error while deleting snapshot on device '%s': %s\n",
-                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
-        error_free(local_err);
-        goto the_end;
-    }
-
     /* save the VM state */
     f = qemu_fopen_bdrv(bs, 1);
     if (!f) {
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH 08/10] migration: reorder processing in hmp_savevm
  2015-11-10 14:25 [Qemu-devel] [PATCH for 2.5 v7 " Denis V. Lunev
@ 2015-11-10 14:25 ` Denis V. Lunev
  0 siblings, 0 replies; 18+ messages in thread
From: Denis V. Lunev @ 2015-11-10 14:25 UTC (permalink / raw)
  Cc: Denis V. Lunev, qemu-devel, Juan Quintela

State deletion can be performed on running VM which reduces VM downtime
This approach looks a bit more natural.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
---
 migration/savevm.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index c2d677d..f4da064 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1267,6 +1267,15 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         return;
     }
 
+    /* Delete old snapshots of the same name */
+    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
+        monitor_printf(mon,
+                       "Error while deleting snapshot on device '%s': %s\n",
+                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
+        error_free(local_err);
+        return;
+    }
+
     bs = find_vmstate_bs();
     if (!bs) {
         monitor_printf(mon, "No block device can accept snapshots\n");
@@ -1304,15 +1313,6 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
     }
 
-    /* Delete old snapshots of the same name */
-    if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
-        monitor_printf(mon,
-                       "Error while deleting snapshot on device '%s': %s\n",
-                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
-        error_free(local_err);
-        goto the_end;
-    }
-
     /* save the VM state */
     f = qemu_fopen_bdrv(bs, 1);
     if (!f) {
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2015-11-16 15:24 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-07 15:54 [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Denis V. Lunev
2015-11-07 15:54 ` [Qemu-devel] [PATCH 01/10] snapshot: create helper to test that block drivers supports snapshots Denis V. Lunev
2015-11-07 15:54 ` [Qemu-devel] [PATCH 02/10] snapshot: return error code from bdrv_snapshot_delete_by_id_or_name Denis V. Lunev
2015-11-07 15:54 ` [Qemu-devel] [PATCH 03/10] snapshot: create bdrv_all_delete_snapshot helper Denis V. Lunev
2015-11-07 15:54 ` [Qemu-devel] [PATCH 04/10] snapshot: create bdrv_all_goto_snapshot helper Denis V. Lunev
2015-11-07 15:54 ` [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper Denis V. Lunev
2015-11-09 17:27   ` Stefan Hajnoczi
2015-11-07 15:54 ` [Qemu-devel] [PATCH 06/10] migration: drop find_vmstate_bs check in hmp_delvm Denis V. Lunev
2015-11-07 15:54 ` [Qemu-devel] [PATCH 07/10] snapshot: create bdrv_all_create_snapshot helper Denis V. Lunev
2015-11-09 17:33   ` Stefan Hajnoczi
2015-11-07 15:54 ` [Qemu-devel] [PATCH 08/10] migration: reorder processing in hmp_savevm Denis V. Lunev
2015-11-07 15:54 ` [Qemu-devel] [PATCH 09/10] migration: implement bdrv_all_find_vmstate_bs helper Denis V. Lunev
2015-11-07 15:55 ` [Qemu-devel] [PATCH 10/10] migration: normalize locking in migration/savevm.c Denis V. Lunev
2015-11-09 17:37 ` [Qemu-devel] [PATCH for 2.5 v6 0/10] dataplane snapshot fixes Stefan Hajnoczi
2015-11-09 17:57   ` Denis V. Lunev
2015-11-10  9:45     ` Stefan Hajnoczi
2015-11-10 14:25 [Qemu-devel] [PATCH for 2.5 v7 " Denis V. Lunev
2015-11-10 14:25 ` [Qemu-devel] [PATCH 08/10] migration: reorder processing in hmp_savevm Denis V. Lunev
2015-11-16 15:24 [Qemu-devel] [PATCH for 2.5 v8 0/10] dataplane snapshot fixes Denis V. Lunev
2015-11-16 15:24 ` [Qemu-devel] [PATCH 08/10] migration: reorder processing in hmp_savevm Denis V. Lunev

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.