[Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes

[Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes

[Denis V. Lunev]

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 v6:
- tricky part dropped from patch 7
- patch 5 reworked to process snapshot list differently in info commands
  and on savevm

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         | 135 ++++++++++++++++++++++++++++++-
 include/block/snapshot.h |  25 +++++-
 migration/savevm.c       | 207 +++++++++++++++--------------------------------
 3 files changed, 217 insertions(+), 150 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 01/10] snapshot: create helper to test that block drivers supports snapshots

[Denis V. Lunev]

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

[Qemu-devel] [PATCH 02/10] snapshot: return error code from bdrv_snapshot_delete_by_id_or_name

[Denis V. Lunev]

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

[Qemu-devel] [PATCH 03/10] snapshot: create bdrv_all_delete_snapshot helper

[Denis V. Lunev]

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

[Qemu-devel] [PATCH 04/10] snapshot: create bdrv_all_goto_snapshot helper

[Denis V. Lunev]

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

[Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Denis V. Lunev]

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         | 21 ++++++++++++++++++
 include/block/snapshot.h |  2 ++
 migration/savevm.c       | 55 +++++++++++++-----------------------------------
 3 files changed, 38 insertions(+), 40 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 9f07a63..97dc315 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -423,3 +423,24 @@ 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, bool read_only,
+                           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 (read_only || (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..0fae32b 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -85,5 +85,7 @@ 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, bool read_only,
+                           BlockDriverState **first_bad_bs);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index d18ff13..90aa565 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, false, &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, true, &bs1) == 0) {
             available_snapshots[total] = i;
             total++;
         }
-- 
2.5.0

[Qemu-devel] [PATCH 06/10] migration: drop find_vmstate_bs check in hmp_delvm

[Denis V. Lunev]

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 90aa565..4c652f3 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

[Qemu-devel] [PATCH 07/10] snapshot: create bdrv_all_create_snapshot helper

[Denis V. Lunev]

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         | 26 ++++++++++++++++++++++++++
 include/block/snapshot.h |  4 ++++
 migration/savevm.c       | 17 ++++-------------
 3 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 97dc315..6de53cb 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -444,3 +444,29 @@ int bdrv_all_find_snapshot(const char *name, bool read_only,
     *first_bad_bs = bs;
     return err;
 }
+
+int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
+                             BlockDriverState *vm_state_bs,
+                             uint64_t vm_state_size,
+                             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 (bs == vm_state_bs) {
+            sn->vm_state_size = vm_state_size;
+            err = bdrv_snapshot_create(bs, sn);
+        } else if (bdrv_can_snapshot(bs)) {
+            sn->vm_state_size = 0;
+            err = bdrv_snapshot_create(bs, sn);
+        }
+        aio_context_release(ctx);
+    }
+
+    *first_bad_bs = bs;
+    return err;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 0fae32b..5f43c0b 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -87,5 +87,9 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
 int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
 int bdrv_all_find_snapshot(const char *name, bool read_only,
                            BlockDriverState **first_bad_bs);
+int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
+                             BlockDriverState *vm_state_bs,
+                             uint64_t vm_state_size,
+                             BlockDriverState **first_bad_bs);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 4c652f3..c2d677d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1328,19 +1328,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, vm_state_size, &bs);
+    if (ret < 0) {
+        monitor_printf(mon, "Error while creating snapshot on '%s'\n",
+                       bdrv_get_device_name(bs));
     }
 
  the_end:
-- 
2.5.0

[Qemu-devel] [PATCH 08/10] migration: reorder processing in hmp_savevm

[Denis V. Lunev]

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

[Qemu-devel] [PATCH 09/10] migration: implement bdrv_all_find_vmstate_bs helper

[Denis V. Lunev]

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 6de53cb..addb7c9 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -470,3 +470,18 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
     *first_bad_bs = 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 5f43c0b..a42a694 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -92,4 +92,6 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
                              uint64_t vm_state_size,
                              BlockDriverState **first_bad_bs);
 
+BlockDriverState *bdrv_all_find_vmstate_bs(void);
+
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index f4da064..20c95b2 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;
@@ -1276,8 +1265,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;
     }
@@ -1386,7 +1375,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;
@@ -1455,7 +1444,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

[Qemu-devel] [PATCH 10/10] migration: normalize locking in migration/savevm.c

[Denis V. Lunev]

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 20c95b2..01110a4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1249,6 +1249,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 "
@@ -1270,6 +1271,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();
 
@@ -1280,6 +1282,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 */
@@ -1324,6 +1328,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     }
 
  the_end:
+    aio_context_release(aio_context);
     if (saved_vm_running) {
         vm_start();
     }
@@ -1362,6 +1367,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.",
@@ -1380,9 +1386,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) {
@@ -1410,9 +1419,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);
@@ -1443,14 +1455,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

Re: [Qemu-devel] [PATCH 04/10] snapshot: create bdrv_all_goto_snapshot helper

[Fam Zheng]

On Tue, 11/10 17:25, Denis V. Lunev wrote:
> 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;

Maybe more friendlily strerror(ret)?

>      }
>  
>      /* restore the VM state */
> -- 
> 2.5.0
> 
> 

Re: [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes

[Fam Zheng]

On Tue, 11/10 17:25, Denis V. Lunev wrote:
> 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         | 135 ++++++++++++++++++++++++++++++-
>  include/block/snapshot.h |  25 +++++-
>  migration/savevm.c       | 207 +++++++++++++++--------------------------------
>  3 files changed, 217 insertions(+), 150 deletions(-)

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 04/10] snapshot: create bdrv_all_goto_snapshot helper

[Denis V. Lunev]

On 11/12/2015 11:38 AM, Fam Zheng wrote:
> On Tue, 11/10 17:25, Denis V. Lunev wrote:
>> 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;
> Maybe more friendlily strerror(ret)?
>
>>       }
>>   
>>       /* restore the VM state */
>> -- 
>> 2.5.0
>>
>>
we can. I think that this could be done for all such functions
as follow up patch. I'll do that separately and will send
with the next iteration if applicable.

Den

Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Stefan Hajnoczi]

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

On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote:
> +int bdrv_all_find_snapshot(const char *name, bool read_only,
> +                           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 (read_only || (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;
> +}

It's difficult to see how bdrv_all_find_snapshot(read_only=true) is
equivalent to what you replaced below:

> @@ -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, true, &bs1) == 0) {

The original loop skips drives that cannot snapshot and the vmstate
drive.  The new code tries to find a snapshot all devices.

To me it seems the semantics are changed.  Can you explain why this
change is safe?

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

Re: [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes

[Stefan Hajnoczi]

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

On Tue, Nov 10, 2015 at 05:25:25PM +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 v6:
> - tricky part dropped from patch 7
> - patch 5 reworked to process snapshot list differently in info commands
>   and on savevm
> 
> 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         | 135 ++++++++++++++++++++++++++++++-
>  include/block/snapshot.h |  25 +++++-
>  migration/savevm.c       | 207 +++++++++++++++--------------------------------
>  3 files changed, 217 insertions(+), 150 deletions(-)

All patches except Patch 6:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Denis V. Lunev]

On 11/16/2015 12:31 PM, Stefan Hajnoczi wrote:
> On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote:
>> +int bdrv_all_find_snapshot(const char *name, bool read_only,
>> +                           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 (read_only || (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;
>> +}
> It's difficult to see how bdrv_all_find_snapshot(read_only=true) is
> equivalent to what you replaced below:
>
>> @@ -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, true, &bs1) == 0) {
> The original loop skips drives that cannot snapshot and the vmstate
> drive.  The new code tries to find a snapshot all devices.
>
> To me it seems the semantics are changed.  Can you explain why this
> change is safe?
yep. you are once again right... OK.

Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Denis V. Lunev]

On 11/16/2015 12:31 PM, Stefan Hajnoczi wrote:
> On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote:
>> +int bdrv_all_find_snapshot(const char *name, bool read_only,
>> +                           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 (read_only || (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;
>> +}
> It's difficult to see how bdrv_all_find_snapshot(read_only=true) is
> equivalent to what you replaced below:
>
>> @@ -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, true, &bs1) == 0) {
> The original loop skips drives that cannot snapshot and the vmstate
> drive.  The new code tries to find a snapshot all devices.
>
> To me it seems the semantics are changed.  Can you explain why this
> change is safe?
argh... This code is used for 'virsh qemu-monitor-command  --hmp rhel7 
info snapshots'
and NOT used for 'virsh snapshot-list rhel7' or used in a very different 
way.

This is the root of the problem.

You are right, the command is broken completely. Some refactoring is 
necessary
here.

Den

[Qemu-devel] [PATCH 06/10] migration: drop find_vmstate_bs check in hmp_delvm

[Denis V. Lunev]

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

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 | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index a391b3a..68af80e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2116,11 +2116,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

[Qemu-devel] [PATCH 06/10] migration: drop find_vmstate_bs check in hmp_delvm

[Denis V. Lunev]

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