[Qemu-devel] [PATCH 2.5 v5 0/11] dataplane snapshot fixes

[Qemu-devel] [PATCH 2.5 v5 0/11] 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 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 (11):
  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
  migration: reorder processing in hmp_savevm
  migration: implement bdrv_all_find_vmstate_bs and bdrv_unlock helpers
  migration: add missed aio_context_acquire for state writing/reading
  snapshot: create bdrv_all_create_snapshot helper
  monitor: add missed aio_context_acquire into vm_completion call

 block/snapshot.c         | 141 +++++++++++++++++++++++++++++-
 include/block/snapshot.h |  23 ++++-
 migration/savevm.c       | 219 +++++++++++++++--------------------------------
 monitor.c                |  11 ++-
 4 files changed, 234 insertions(+), 160 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 01/11] 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>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/snapshot.c         | 27 +++++++++++++++++++++++++++
 include/block/snapshot.h |  9 +++++++++
 migration/savevm.c       | 17 ++++-------------
 3 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 89500f2..2ef4545 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -356,3 +356,30 @@ 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 dataplace (take aio_context_acquire
+ * when appropriate for appropriate block drivers
+ *
+ * Returned block driver will be always locked.
+ */
+
+bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
+{
+    bool ok = true;
+    BlockDriverState *bs = NULL;
+
+    while ((bs = bdrv_next(bs)) && ok) {
+        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..0a3cf0d 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -75,4 +75,13 @@ 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 dataplace (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 dbcc39a..3ac3f07 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

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

[Greg Kurz]

On Wed,  4 Nov 2015 20:19:32 +0300
"Denis V. Lunev" <den@openvz.org> wrote:

> The patch enforces proper locking for this 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         | 27 +++++++++++++++++++++++++++
>  include/block/snapshot.h |  9 +++++++++
>  migration/savevm.c       | 17 ++++-------------
>  3 files changed, 40 insertions(+), 13 deletions(-)
> 
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 89500f2..2ef4545 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -356,3 +356,30 @@ 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 dataplace (take aio_context_acquire

s/dataplace/dataplane

> + * when appropriate for appropriate block drivers

Missing ) and anyway the "(take aio_context_acquire..." part more or less
quotes the code and is not needed.

> + *
> + * Returned block driver will be always locked.
> + */
> +
> +bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)

Juan suggested bdrv_snapshot_is_possible() and FWIW you already emphasize
that it involves all block drivers in the above comment.

> +{
> +    bool ok = true;
> +    BlockDriverState *bs = NULL;
> +
> +    while ((bs = bdrv_next(bs)) && ok) {
> +        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..0a3cf0d 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -75,4 +75,13 @@ 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 dataplace (take aio_context_acquire
> + * when appropriate for appropriate block drivers
> + *
> + */

This comment is not needed here, already in block/snapshot.c

> +bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
> +
>  #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index dbcc39a..3ac3f07 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();

Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

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

[Denis V. Lunev]

On 11/05/2015 09:26 AM, Greg Kurz wrote:
> On Wed,  4 Nov 2015 20:19:32 +0300
> "Denis V. Lunev" <den@openvz.org> wrote:
>
>> The patch enforces proper locking for this 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         | 27 +++++++++++++++++++++++++++
>>   include/block/snapshot.h |  9 +++++++++
>>   migration/savevm.c       | 17 ++++-------------
>>   3 files changed, 40 insertions(+), 13 deletions(-)
>>
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index 89500f2..2ef4545 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -356,3 +356,30 @@ 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 dataplace (take aio_context_acquire
> s/dataplace/dataplane
>
>> + * when appropriate for appropriate block drivers
> Missing ) and anyway the "(take aio_context_acquire..." part more or less
> quotes the code and is not needed.
>
>> + *
>> + * Returned block driver will be always locked.
>> + */
>> +
>> +bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
> Juan suggested bdrv_snapshot_is_possible() and FWIW you already emphasize
> that it involves all block drivers in the above comment.
>
>> +{
>> +    bool ok = true;
>> +    BlockDriverState *bs = NULL;
>> +
>> +    while ((bs = bdrv_next(bs)) && ok) {
>> +        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..0a3cf0d 100644
>> --- a/include/block/snapshot.h
>> +++ b/include/block/snapshot.h
>> @@ -75,4 +75,13 @@ 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 dataplace (take aio_context_acquire
>> + * when appropriate for appropriate block drivers
>> + *
>> + */
> This comment is not needed here, already in block/snapshot.c
it is better to have it twice.


>> +bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
>> +
>>   #endif
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index dbcc39a..3ac3f07 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();
> Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>
Thank you for spelling check.

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

[Stefan Hajnoczi]

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

On Thu, Nov 05, 2015 at 07:26:50AM +0100, Greg Kurz wrote:
> On Wed,  4 Nov 2015 20:19:32 +0300
> > + *
> > + * Returned block driver will be always locked.
> > + */
> > +
> > +bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
> 
> Juan suggested bdrv_snapshot_is_possible() and FWIW you already emphasize
> that it involves all block drivers in the above comment.

I like the "all" in the function name.  There is a set of bdrv_*_all()
functions that operate on all BlockDriverStates.  What these functions
have in common is that they acquire AioContext.  Other bdrv_*()
functions don't do this, so the naming helps make the semantics clear.

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

[Qemu-devel] [PATCH 02/11] 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 2ef4545..d729c76 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 0a3cf0d..76cf6a3 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

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

[Greg Kurz]

On Wed,  4 Nov 2015 20:19:33 +0300
"Denis V. Lunev" <den@openvz.org> wrote:

> this will make code better in the next patch
> 

This changelog is not very useful. IMHO this calls for squashing this patch
into the next one: then we clearly see how the return value is used and why
the code is better.

> 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 2ef4545..d729c76 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 0a3cf0d..76cf6a3 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,

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

[Denis V. Lunev]

On 11/05/2015 10:55 AM, Greg Kurz wrote:
> On Wed,  4 Nov 2015 20:19:33 +0300
> "Denis V. Lunev" <den@openvz.org> wrote:
>
>> this will make code better in the next patch
>>
> This changelog is not very useful. IMHO this calls for squashing this patch
> into the next one: then we clearly see how the return value is used and why
> the code is better.

is it principal to get the series merged?

In general I have a strong opinion that non-functional changes
should be separated from functional ones. This makes review
faster and allows to concentrate on things where we can have
mistakes.

Den

[Qemu-devel] [PATCH 03/11] 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 d729c76..1b4b846 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -384,3 +384,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 ((bs = bdrv_next(bs)) && ret == 0) {
+        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 76cf6a3..9c9750a 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -83,5 +83,7 @@ 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);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 3ac3f07..5a4a8c2 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

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

[Stefan Hajnoczi]

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

On Wed, Nov 04, 2015 at 08:19:34PM +0300, Denis V. Lunev wrote:
> 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 d729c76..1b4b846 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -384,3 +384,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 ((bs = bdrv_next(bs)) && ret == 0) {

If ret != 0 we will iterate to the next bs.  first_bad_bs will be
incorrect.

> +        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;
> +}

Similar approach without the int return value:

bool bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
                              Error **err)
{
    Error *local_err = NULL;
    BlockDriverState *bs = NULL;
    QEMUSnapshotInfo sn1, *snapshot = &sn1;

    while ((bs = bdrv_next(bs)) {
        AioContext *ctx = bdrv_get_aio_context(bs);

	aio_context_acquire(bs);
	if (bdrv_can_snapshot(bs) &&
	    bdrv_snapshot_find(bs, snapshot, name) >= 0) {
	    bdrv_snapshot_delete_by_id_or_name(bs, name, &local_err);
	}
	aio_context_release(bs);

	if (local_err) {
	    error_propagate(err, local_err);
	    return false;
	}
    }
    return true;
}

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

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

[Denis V. Lunev]

On 11/06/2015 05:09 PM, Stefan Hajnoczi wrote:
> On Wed, Nov 04, 2015 at 08:19:34PM +0300, Denis V. Lunev wrote:
>> 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 d729c76..1b4b846 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -384,3 +384,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 ((bs = bdrv_next(bs)) && ret == 0) {
> If ret != 0 we will iterate to the next bs.  first_bad_bs will be
> incorrect.
good catch! this applies as well to all patches below.
You don't need to point this again :)

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

[Denis V. Lunev]

On 11/06/2015 05:09 PM, Stefan Hajnoczi wrote:
> On Wed, Nov 04, 2015 at 08:19:34PM +0300, Denis V. Lunev wrote:
>> 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 d729c76..1b4b846 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -384,3 +384,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 ((bs = bdrv_next(bs)) && ret == 0) {
> If ret != 0 we will iterate to the next bs.  first_bad_bs will be
> incorrect.
>
>> +        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;
>> +}
> Similar approach without the int return value:
>
> bool bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
>                                Error **err)
> {
>      Error *local_err = NULL;
>      BlockDriverState *bs = NULL;
>      QEMUSnapshotInfo sn1, *snapshot = &sn1;
>
>      while ((bs = bdrv_next(bs)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
>
> 	aio_context_acquire(bs);
> 	if (bdrv_can_snapshot(bs) &&
> 	    bdrv_snapshot_find(bs, snapshot, name) >= 0) {
> 	    bdrv_snapshot_delete_by_id_or_name(bs, name, &local_err);
> 	}
> 	aio_context_release(bs);
>
> 	if (local_err) {
> 	    error_propagate(err, local_err);
> 	    return false;
> 	}
>      }
>      return true;
> }
there are 2 notes.

Personally I do not like 'bool' functions as it is unclear
whether true means success or failure. Usually false is
failure but I have faced awkward situation when conditions
where reversed.

<0 and >=0 conditions are alive for 10th of years.

secondly the you will have to assign first_bad_bs two times.
I have started from this version of code and merged them
later to the current approach. Ok, I have made a mistake :(

Den

[Qemu-devel] [PATCH 04/11] 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>
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 1b4b846..639074f 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -406,3 +406,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 ((bs = bdrv_next(bs)) && err == 0) {
+        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 9c9750a..9c54b7c 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -85,5 +85,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 5a4a8c2..101dff5 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

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

[Greg Kurz]

On Wed,  4 Nov 2015 20:19:35 +0300
"Denis V. Lunev" <den@openvz.org> 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>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---

Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.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 1b4b846..639074f 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -406,3 +406,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;

Cosmetic comment again :)

What about s/err/ret to be consistent with other functions in block/snapshot.c ?

> +    BlockDriverState *bs = NULL;
> +
> +    while ((bs = bdrv_next(bs)) && err == 0) {
> +        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 9c9750a..9c54b7c 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -85,5 +85,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 5a4a8c2..101dff5 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 */

[Qemu-devel] [PATCH 05/11] 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         | 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 639074f..bb3bfd5 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -426,3 +426,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 ((bs = bdrv_next(bs)) && err == 0) {
+        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 9c54b7c..44c3a38 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -86,5 +86,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 101dff5..4e469ef 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

[Qemu-devel] [PATCH 06/11] 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 4e469ef..f7ff37a 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/11] migration: reorder processing in hmp_savevm

[Denis V. Lunev]

This approach looks a bit more natural.

State deletion can be performed on running VM which reduces VM downtime

The situation when we have all devices read-only is not that frequent
to optimize for it. We can afford to pause VM for the case. This reduces
the amount of time when aio_context remains locked with next patches.

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

diff --git a/migration/savevm.c b/migration/savevm.c
index f7ff37a..dbcb313 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1267,9 +1267,12 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    bs = find_vmstate_bs();
-    if (!bs) {
-        monitor_printf(mon, "No block device can accept snapshots\n");
+    /* Delete old snapshots of the same name */
+    if (name && bdrv_all_delete_snapshot(name, &bs, &local_err) < 0) {
+        monitor_printf(mon,
+                       "Error while deleting snapshot on device '%s': %s\n",
+                       bdrv_get_device_name(bs), error_get_pretty(local_err));
+        error_free(local_err);
         return;
     }
 
@@ -1290,6 +1293,12 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     sn->date_nsec = tv.tv_usec * 1000;
     sn->vm_clock_nsec = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 
+    bs = find_vmstate_bs();
+    if (!bs) {
+        monitor_printf(mon, "No block device can accept snapshots\n");
+        goto the_end;
+    }
+
     if (name) {
         ret = bdrv_snapshot_find(bs, old_sn, name);
         if (ret >= 0) {
@@ -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 08/11] migration: implement bdrv_all_find_vmstate_bs and bdrv_unlock helpers

[Denis V. Lunev]

bdrv_all_find_vmstate_bs will return locked BlockDriverState. The code
written in the way to avoid nested locking taking into account that
patch for locking inside qemu_fopen_bdrv/qemu_fclose is acceptable.

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 |  3 +++
 migration/savevm.c       | 23 ++++++++---------------
 3 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index bb3bfd5..e196c9b 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -446,3 +446,24 @@ int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
     *first_bad_bs = bs;
     return err;
 }
+
+BlockDriverState *bdrv_all_find_vmstate_bs(void)
+{
+    BlockDriverState *bs = NULL;
+
+    while ((bs = bdrv_next(bs))) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
+        if (bdrv_can_snapshot(bs)) {
+            return bs;
+        }
+        aio_context_release(ctx);
+    }
+    return NULL;
+}
+
+void bdrv_unlock(BlockDriverState *bs)
+{
+    aio_context_release(bdrv_get_aio_context(bs));
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 44c3a38..1f45f51 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -88,4 +88,7 @@ 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, BlockDriverState **first_bad_bs);
 
+BlockDriverState *bdrv_all_find_vmstate_bs(void);
+void bdrv_unlock(BlockDriverState *bs);
+
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index dbcb313..9339f2e 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;
@@ -1293,8 +1282,8 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     sn->date_nsec = tv.tv_usec * 1000;
     sn->vm_clock_nsec = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 
-    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");
         goto the_end;
     }
@@ -1312,6 +1301,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         localtime_r((const time_t *)&tv.tv_sec, &tm);
         strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
     }
+    bdrv_unlock(bs);
 
     /* save the VM state */
     f = qemu_fopen_bdrv(bs, 1);
@@ -1395,7 +1385,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;
@@ -1403,6 +1393,8 @@ int load_vmstate(const char *name)
 
     /* Don't even try to load empty VM states */
     ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
+    bdrv_unlock(bs_vm_state);
+
     if (ret < 0) {
         return ret;
     } else if (sn.vm_state_size == 0) {
@@ -1464,13 +1456,14 @@ 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;
     }
 
     nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    bdrv_unlock(bs);
     if (nb_sns < 0) {
         monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
         return;
-- 
2.5.0

Re: [Qemu-devel] [PATCH 08/11] migration: implement bdrv_all_find_vmstate_bs and bdrv_unlock helpers

[Stefan Hajnoczi]

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

On Wed, Nov 04, 2015 at 08:19:39PM +0300, Denis V. Lunev wrote:
> +BlockDriverState *bdrv_all_find_vmstate_bs(void)
> +{
> +    BlockDriverState *bs = NULL;
> +
> +    while ((bs = bdrv_next(bs))) {
> +        AioContext *ctx = bdrv_get_aio_context(bs);
> +
> +        aio_context_acquire(ctx);
> +        if (bdrv_can_snapshot(bs)) {
> +            return bs;

This leaves AioContext acquired.  If that is intentional then it must be
documented because it looks like a bug.

Normally functions that do this have an AioContext **aio_context agument
so the caller does aio_context_release() later.  This way it's obvious
that the caller needs to release.

For example, see blockdev.c:find_block_job().

> +        }
> +        aio_context_release(ctx);
> +    }
> +    return NULL;
> +}
> +
> +void bdrv_unlock(BlockDriverState *bs)
> +{
> +    aio_context_release(bdrv_get_aio_context(bs));
> +}

This API is weird.  There is no lock function.  Please do what I
mentioned above.

Another advantage of that approach is that we are 100% sure to release
the same AioContext that was acquired (even if bdrv_set_aio_context()
gets called halfway through).

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

Re: [Qemu-devel] [PATCH 08/11] migration: implement bdrv_all_find_vmstate_bs and bdrv_unlock helpers

[Denis V. Lunev]

On 11/06/2015 06:18 PM, Stefan Hajnoczi wrote:
> On Wed, Nov 04, 2015 at 08:19:39PM +0300, Denis V. Lunev wrote:
>> +BlockDriverState *bdrv_all_find_vmstate_bs(void)
>> +{
>> +    BlockDriverState *bs = NULL;
>> +
>> +    while ((bs = bdrv_next(bs))) {
>> +        AioContext *ctx = bdrv_get_aio_context(bs);
>> +
>> +        aio_context_acquire(ctx);
>> +        if (bdrv_can_snapshot(bs)) {
>> +            return bs;
> This leaves AioContext acquired.  If that is intentional then it must be
> documented because it looks like a bug.
>
> Normally functions that do this have an AioContext **aio_context agument
> so the caller does aio_context_release() later.  This way it's obvious
> that the caller needs to release.
>
> For example, see blockdev.c:find_block_job().
>
>> +        }
>> +        aio_context_release(ctx);
>> +    }
>> +    return NULL;
>> +}
>> +
>> +void bdrv_unlock(BlockDriverState *bs)
>> +{
>> +    aio_context_release(bdrv_get_aio_context(bs));
>> +}
> This API is weird.  There is no lock function.  Please do what I
> mentioned above.
>
> Another advantage of that approach is that we are 100% sure to release
> the same AioContext that was acquired (even if bdrv_set_aio_context()
> gets called halfway through).
no prob if Juan will accept that :) Ho does not want to care
and take the lock anywhere in his code. For me this is
pure matter of taste.

[Qemu-devel] [PATCH 09/11] migration: add missed aio_context_acquire for state writing/reading

[Denis V. Lunev]

aio_context should be locked in the similar way as was done in QMP
snapshot creation in the other case there are a lot of possible
troubles if native AIO mode is enabled for disk.

qemu_fopen_bdrv and bdrv_fclose are used in real snapshot operations only
along with block drivers. This change should influence only HMP snapshot
operations.

AioContext lock is reqursive. Thus nested locking should not be a problem.

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

diff --git a/migration/savevm.c b/migration/savevm.c
index 9339f2e..f8c727d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -153,7 +153,11 @@ static ssize_t block_get_buffer(void *opaque, uint8_t *buf, int64_t pos,
 
 static int bdrv_fclose(void *opaque)
 {
-    return bdrv_flush(opaque);
+    BlockDriverState *bs = (BlockDriverState *)opaque;
+    int ret = bdrv_flush(bs);
+
+    aio_context_release(bdrv_get_aio_context(bs));
+    return ret;
 }
 
 static const QEMUFileOps bdrv_read_ops = {
@@ -169,10 +173,18 @@ static const QEMUFileOps bdrv_write_ops = {
 
 static QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int is_writable)
 {
+    QEMUFile *file;
+
     if (is_writable) {
-        return qemu_fopen_ops(bs, &bdrv_write_ops);
+        file = qemu_fopen_ops(bs, &bdrv_write_ops);
+    } else {
+        file = qemu_fopen_ops(bs, &bdrv_read_ops);
+    }
+
+    if (file != NULL) {
+        aio_context_acquire(bdrv_get_aio_context(bs));
     }
-    return qemu_fopen_ops(bs, &bdrv_read_ops);
+    return file;
 }
 
 
-- 
2.5.0

Re: [Qemu-devel] [PATCH 09/11] migration: add missed aio_context_acquire for state writing/reading

[Stefan Hajnoczi]

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

On Wed, Nov 04, 2015 at 08:19:40PM +0300, Denis V. Lunev wrote:
> aio_context should be locked in the similar way as was done in QMP
> snapshot creation in the other case there are a lot of possible
> troubles if native AIO mode is enabled for disk.
> 
> qemu_fopen_bdrv and bdrv_fclose are used in real snapshot operations only
> along with block drivers. This change should influence only HMP snapshot
> operations.
> 
> AioContext lock is reqursive. Thus nested locking should not be a problem.

hmp_savevm() and load_vmstate() look up the BlockDriverState and perform
other operations on it.  So the natural place to call acquire/release is
in hmp_savevm() and load_vmstate().  They need that anyway since they
also perform other operations.

qemu_fopen_bdrv() and bdrv_fclose() don't need to worry about
AioContext.

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

[Qemu-devel] [PATCH 10/11] 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         | 24 ++++++++++++++++++++++++
 include/block/snapshot.h |  1 +
 migration/savevm.c       | 22 ++++++----------------
 3 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index e196c9b..15d2b8a 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -456,6 +456,8 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)
 
         aio_context_acquire(ctx);
         if (bdrv_can_snapshot(bs)) {
+            /* Do not change logic here without tweaking
+             * bdrv_all_create_snapshot below */
             return bs;
         }
         aio_context_release(ctx);
@@ -467,3 +469,25 @@ void bdrv_unlock(BlockDriverState *bs)
 {
     aio_context_release(bdrv_get_aio_context(bs));
 }
+
+int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn, BlockDriverState **bad)
+{
+    int err = 0;
+    BlockDriverState *bs = NULL;
+
+    while ((bs = bdrv_next(bs)) && err == 0) {
+        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 1f45f51..0a29b2d 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -90,5 +90,6 @@ int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
 
 BlockDriverState *bdrv_all_find_vmstate_bs(void);
 void bdrv_unlock(BlockDriverState *bs);
+int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn, BlockDriverState **bad);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index f8c727d..8db7895 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1251,12 +1251,11 @@ out:
 
 void hmp_savevm(Monitor *mon, const QDict *qdict)
 {
-    BlockDriverState *bs, *bs1;
+    BlockDriverState *bs;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
     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");
@@ -1322,7 +1321,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));
@@ -1330,19 +1329,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

[Qemu-devel] [PATCH 11/11] monitor: add missed aio_context_acquire into vm_completion call

[Denis V. Lunev]

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Luiz Capitulino <lcapitulino@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 6cd747f..3295840 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3408,13 +3408,18 @@ static void vm_completion(ReadLineState *rs, const char *str)
     readline_set_completion_index(rs, len);
     while ((bs = bdrv_next(bs))) {
         SnapshotInfoList *snapshots, *snapshot;
+        AioContext *ctx = bdrv_get_aio_context(bs);
+        bool ok = false;
 
-        if (!bdrv_can_snapshot(bs)) {
-            continue;
+        aio_context_acquire(ctx);
+        if (bdrv_can_snapshot(bs)) {
+            ok = bdrv_query_snapshot_info_list(bs, &snapshots, NULL) == 0;
         }
-        if (bdrv_query_snapshot_info_list(bs, &snapshots, NULL)) {
+        aio_context_release(ctx);
+        if (!ok) {
             continue;
         }
+
         snapshot = snapshots;
         while (snapshot) {
             char *completion = snapshot->value->name;
-- 
2.5.0

Re: [Qemu-devel] [PATCH 11/11] monitor: add missed aio_context_acquire into vm_completion call

[Stefan Hajnoczi]

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

On Wed, Nov 04, 2015 at 08:19:42PM +0300, Denis V. Lunev wrote:
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Luiz Capitulino <lcapitulino@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Thanks, applied this patch by itself to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

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

Re: [Qemu-devel] [PATCH 2.5 v5 0/11] dataplane snapshot fixes

[Stefan Hajnoczi]

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

On Wed, Nov 04, 2015 at 08:19:31PM +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 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 (11):
>   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
>   migration: reorder processing in hmp_savevm
>   migration: implement bdrv_all_find_vmstate_bs and bdrv_unlock helpers
>   migration: add missed aio_context_acquire for state writing/reading
>   snapshot: create bdrv_all_create_snapshot helper
>   monitor: add missed aio_context_acquire into vm_completion call
> 
>  block/snapshot.c         | 141 +++++++++++++++++++++++++++++-
>  include/block/snapshot.h |  23 ++++-
>  migration/savevm.c       | 219 +++++++++++++++--------------------------------
>  monitor.c                |  11 ++-
>  4 files changed, 234 insertions(+), 160 deletions(-)

There is a lot going on here.  For the 2.5 release, I'd prefer to limit
changes to fixing bugs like in the last patch (which I've merged) and
refactoring to a minimum (because that's where there will be discussions
and it may take a while).

By the way, the reason these issues haven't been noticed is that savevm
isn't used much by libvirt users (backing files are used for snapshots
instead).  That means the savevm code is only called from live migration
code paths.

Dataplane temporarily disables itself during live migration so no
problems are hit in that case.

The HMP monitor is legacy and also not used by modern libvirt.

I think the affected use cases are restricted to savevm+dataplane and
HMP+dataplane.

Stefan

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

Re: [Qemu-devel] [PATCH 2.5 v5 0/11] dataplane snapshot fixes

[Eric Blake]

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

On 11/06/2015 08:54 AM, Stefan Hajnoczi wrote:
> On Wed, Nov 04, 2015 at 08:19:31PM +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.

That is a case of using libvirt to trigger internal snapshots...

> 
> The HMP monitor is legacy and also not used by modern libvirt.

...and libvirt is forced to use HMP for internal snapshots, since we
_still_ haven't exposed internal snapshots as a QMP command.

> I think the affected use cases are restricted to savevm+dataplane and
> HMP+dataplane.

The fact that the commit message calls out a libvirt method of
triggering the bug does mean that it is user-visible, and so it would
qualify as a bug fix even during hard freeze.  But I also understand
that taking a large complex series late in the game is not without risk;
and it is not like this is a regression (rather, something that has
never worked bulletproof), right?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2.5 v5 0/11] dataplane snapshot fixes

[Denis V. Lunev]

On 11/06/2015 07:05 PM, Eric Blake wrote:
> On 11/06/2015 08:54 AM, Stefan Hajnoczi wrote:
>> On Wed, Nov 04, 2015 at 08:19:31PM +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.
> That is a case of using libvirt to trigger internal snapshots...
>
>> The HMP monitor is legacy and also not used by modern libvirt.
> ...and libvirt is forced to use HMP for internal snapshots, since we
> _still_ haven't exposed internal snapshots as a QMP command.
>
>> I think the affected use cases are restricted to savevm+dataplane and
>> HMP+dataplane.
> The fact that the commit message calls out a libvirt method of
> triggering the bug does mean that it is user-visible, and so it would
> qualify as a bug fix even during hard freeze.  But I also understand
> that taking a large complex series late in the game is not without risk;
> and it is not like this is a regression (rather, something that has
> never worked bulletproof), right?
>
yes, this was not working in the past and this is not a regression.

The problem is that it seems that NOBODY uses iothreads in the
production or even for complex real life production tests. There
is another recently merged example of this (100% reproducible,
happens both on migration/snapshot). We have faced this on
suspend operation.

commit 10a06fd65f667a972848ebbbcac11bdba931b544
Author: Pavel Butsykin <pbutsykin@virtuozzo.com>
Date:   Mon Oct 26 14:42:57 2015 +0300

     virtio: sync the dataplane vring state to the virtqueue before 
virtio_save

I have started this initially as a set of small bits in savevm code
and was asked to move the code from savevm.c to block layer.
This has been done and yes, series becomes complex after
that and it was obvious that it will be complex when the task
was set to move a bunch of code from one place to another.

Anyway, from my point of view the serie is not that complex.
It is just large and is doing simple things almost near copy/paste
and there is a month to catch bugs here.

Can we still consider this for merge?

Den

Re: [Qemu-devel] [PATCH 2.5 v5 0/11] dataplane snapshot fixes

[Stefan Hajnoczi]

On Fri, Nov 6, 2015 at 4:19 PM, Denis V. Lunev <den@openvz.org> wrote:
> On 11/06/2015 07:05 PM, Eric Blake wrote:
>>
>> On 11/06/2015 08:54 AM, Stefan Hajnoczi wrote:
>>>
>>> On Wed, Nov 04, 2015 at 08:19:31PM +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.
>>
>> That is a case of using libvirt to trigger internal snapshots...
>>
>>> The HMP monitor is legacy and also not used by modern libvirt.
>>
>> ...and libvirt is forced to use HMP for internal snapshots, since we
>> _still_ haven't exposed internal snapshots as a QMP command.
>>
>>> I think the affected use cases are restricted to savevm+dataplane and
>>> HMP+dataplane.
>>
>> The fact that the commit message calls out a libvirt method of
>> triggering the bug does mean that it is user-visible, and so it would
>> qualify as a bug fix even during hard freeze.  But I also understand
>> that taking a large complex series late in the game is not without risk;
>> and it is not like this is a regression (rather, something that has
>> never worked bulletproof), right?
>>
> yes, this was not working in the past and this is not a regression.
>
> The problem is that it seems that NOBODY uses iothreads in the
> production or even for complex real life production tests. There
> is another recently merged example of this (100% reproducible,
> happens both on migration/snapshot). We have faced this on
> suspend operation.
>
> commit 10a06fd65f667a972848ebbbcac11bdba931b544
> Author: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Date:   Mon Oct 26 14:42:57 2015 +0300
>
>     virtio: sync the dataplane vring state to the virtqueue before
> virtio_save
>
> I have started this initially as a set of small bits in savevm code
> and was asked to move the code from savevm.c to block layer.
> This has been done and yes, series becomes complex after
> that and it was obvious that it will be complex when the task
> was set to move a bunch of code from one place to another.
>
> Anyway, from my point of view the serie is not that complex.
> It is just large and is doing simple things almost near copy/paste
> and there is a month to catch bugs here.
>
> Can we still consider this for merge?

Absolutely, they are still bugs and we can fix them for 2.5.

I just wanted to reflect on the scope of the bugs and it occurred to
me that these code paths haven't been exercised/tested as often.

Stefan

Re: [Qemu-devel] [PATCH 2.5 v5 0/11] dataplane snapshot fixes

[Denis V. Lunev]

On 11/06/2015 07:05 PM, Eric Blake wrote:
> On 11/06/2015 08:54 AM, Stefan Hajnoczi wrote:
>> On Wed, Nov 04, 2015 at 08:19:31PM +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.
> That is a case of using libvirt to trigger internal snapshots...
>
>> The HMP monitor is legacy and also not used by modern libvirt.
> ...and libvirt is forced to use HMP for internal snapshots, since we
> _still_ haven't exposed internal snapshots as a QMP command.

Eric,

by the way, there is a user visible bug with this too :))))))

EFI based VM with pflash storage for NVRAM could not
be snapshoted as libvirt configures storage as 'raw'.
OK, this is a libvirt bug. The patch will be sent next
week or in a week by my colleague switching storage
type from 'raw' to 'qcow2'.

For a QEMU this results in the following:
- QEMU receives command via HMP to make a snapshot
- it fails, but QEMU does not see that fact (error code
   is not delivered to libvirt in HMP AFAIK)
- on request to switch to snapshot the commands
   just do nothing and from the point of libvirt the command
   was successful

We should have these commands even in the simple
rudimentary current non-ideal form even just as wrappers
around HMP functions.

Do you have an opinion about importance of the last
issue? Should it be considered for 2.6?

Den

Re: [Qemu-devel] [PATCH 2.5 v5 0/11] dataplane snapshot fixes

[Eric Blake]

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

On 11/06/2015 02:13 PM, Denis V. Lunev wrote:

>> That is a case of using libvirt to trigger internal snapshots...
>>
>>> The HMP monitor is legacy and also not used by modern libvirt.
>> ...and libvirt is forced to use HMP for internal snapshots, since we
>> _still_ haven't exposed internal snapshots as a QMP command.
> 
> Eric,
> 
> by the way, there is a user visible bug with this too :))))))
> 
> EFI based VM with pflash storage for NVRAM could not
> be snapshoted as libvirt configures storage as 'raw'.
> OK, this is a libvirt bug. The patch will be sent next
> week or in a week by my colleague switching storage
> type from 'raw' to 'qcow2'.

Not necessarily a bug in libvirt, so much as a limitation that the
current qemu implementation of internal snapshots requires qcow2 for all
storage devices (even though it might not be technically necessary, if
there were an easy way to snapshot state of one non-qcow2 storage
alongside the rest of the machine state stored elsewhere).  But a
libvirt patch would certainly be useful.

> 
> For a QEMU this results in the following:
> - QEMU receives command via HMP to make a snapshot
> - it fails, but QEMU does not see that fact (error code
>   is not delivered to libvirt in HMP AFAIK)

Libvirt is still using QMP to deliver the HMP command (via the QMP
human-monitor-command); if I'm understanding your complaint correctly,
you are saying that qemu doesn't do error reporting correctly for that?
If so, fix that in qemu, although libvirt should also be able to work
around it when dealing with broken qemu.

> - on request to switch to snapshot the commands
>   just do nothing and from the point of libvirt the command
>   was successful
> 
> We should have these commands even in the simple
> rudimentary current non-ideal form even just as wrappers
> around HMP functions.
> 
> Do you have an opinion about importance of the last
> issue? Should it be considered for 2.6?

We've gone since 0.14 without anyone writing the remaining few QMP
commands to completely obsolete the need for human-monitor-command
backdoors into HMP.  Volunteers are welcome to submit code to complete
the conversion, but the length of time it has taken so far may be an
indication that it is not as easy as you think.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2.5 v5 0/11] dataplane snapshot fixes

[Denis V. Lunev]

On 11/10/2015 12:05 AM, Eric Blake wrote:
> On 11/06/2015 02:13 PM, Denis V. Lunev wrote:
>
>>> That is a case of using libvirt to trigger internal snapshots...
>>>
>>>> The HMP monitor is legacy and also not used by modern libvirt.
>>> ...and libvirt is forced to use HMP for internal snapshots, since we
>>> _still_ haven't exposed internal snapshots as a QMP command.
>> Eric,
>>
>> by the way, there is a user visible bug with this too :))))))
>>
>> EFI based VM with pflash storage for NVRAM could not
>> be snapshoted as libvirt configures storage as 'raw'.
>> OK, this is a libvirt bug. The patch will be sent next
>> week or in a week by my colleague switching storage
>> type from 'raw' to 'qcow2'.
> Not necessarily a bug in libvirt, so much as a limitation that the
> current qemu implementation of internal snapshots requires qcow2 for all
> storage devices (even though it might not be technically necessary, if
> there were an easy way to snapshot state of one non-qcow2 storage
> alongside the rest of the machine state stored elsewhere).  But a
> libvirt patch would certainly be useful.
>
>> For a QEMU this results in the following:
>> - QEMU receives command via HMP to make a snapshot
>> - it fails, but QEMU does not see that fact (error code
>>    is not delivered to libvirt in HMP AFAIK)
> Libvirt is still using QMP to deliver the HMP command (via the QMP
> human-monitor-command); if I'm understanding your complaint correctly,
> you are saying that qemu doesn't do error reporting correctly for that?
> If so, fix that in qemu, although libvirt should also be able to work
> around it when dealing with broken qemu.
>
>> - on request to switch to snapshot the commands
>>    just do nothing and from the point of libvirt the command
>>    was successful
>>
>> We should have these commands even in the simple
>> rudimentary current non-ideal form even just as wrappers
>> around HMP functions.
>>
>> Do you have an opinion about importance of the last
>> issue? Should it be considered for 2.6?
> We've gone since 0.14 without anyone writing the remaining few QMP
> commands to completely obsolete the need for human-monitor-command
> backdoors into HMP.  Volunteers are welcome to submit code to complete
> the conversion, but the length of time it has taken so far may be an
> indication that it is not as easy as you think.
>
I have one for snapshot operations :)

Den