[Qemu-devel] [PATCH 0/4] some migration bugs

[Qemu-devel] [PATCH 0/4] some migration bugs

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here are some migration related bugs, two about INACTIVE bdses and one
use-after-free.

I'm absolutely not sure, that these bugs should be fixed like I'm fixing,
but problem definitely exists.

Reset in stopped state is strange case, may be such usage should be
restricted.
About INACTIVE - looks like it should be a separate run-state, not only
bdrv-flag.
Situation with migration state, which is global, but is set/reset/changed
in not controlled manner is not very good too..

Vladimir Sementsov-Ogievskiy (4):
  iotests: add migration corner cases test
  qmp-cont: invalidate on RUN_STATE_PRELAUNCH
  savevm: fix savevm after migration
  migration: fix use-after-free of to_dst_file

 block/snapshot.c           |  3 +-
 migration/savevm.c         | 16 +++++++++++
 qmp.c                      |  3 +-
 tests/qemu-iotests/175     | 71 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/175.out |  5 ++++
 tests/qemu-iotests/group   |  1 +
 6 files changed, 97 insertions(+), 2 deletions(-)
 create mode 100644 tests/qemu-iotests/175
 create mode 100644 tests/qemu-iotests/175.out

-- 
2.11.1

[Qemu-devel] [PATCH 1/4] iotests: add migration corner cases test

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/175     | 71 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/175.out |  5 ++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 77 insertions(+)
 create mode 100644 tests/qemu-iotests/175
 create mode 100644 tests/qemu-iotests/175.out

diff --git a/tests/qemu-iotests/175 b/tests/qemu-iotests/175
new file mode 100644
index 0000000000..ef86c70db5
--- /dev/null
+++ b/tests/qemu-iotests/175
@@ -0,0 +1,71 @@
+#!/usr/bin/env python
+#
+# Test migration corner-cases
+#
+# Copyright (C) Vladimir Sementsov-Ogievskiy 2017
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+import time
+from iotests import qemu_img
+
+disk = os.path.join(iotests.test_dir, 'disk')
+
+class TestMigrationCornerCases(iotests.QMPTestCase):
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, disk, '10M')
+        self.vm = iotests.VM().add_drive(disk)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(disk)
+
+    def test_migrate_reset_cont_write(self):
+        result = self.vm.qmp('migrate', uri='exec:cat>/dev/null')
+        self.assert_qmp(result, 'return', {})
+        time.sleep(4)
+
+        result = self.vm.qmp('human-monitor-command',
+                             command_line='system_reset')
+        self.assert_qmp(result, 'return', '')
+
+        result = self.vm.qmp('cont')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('human-monitor-command',
+                             command_line='qemu-io drive0 "write 0 512"')
+        self.assert_qmp(result, 'return', '')
+
+    def test_migrate_savevm(self):
+        result = self.vm.qmp('migrate', uri='exec:cat>/dev/null')
+        self.assert_qmp(result, 'return', {})
+        time.sleep(4)
+
+        result = self.vm.qmp('human-monitor-command', command_line='savevm')
+        self.assert_qmp(result, 'return', '')
+
+    def test_savevm_set_speed_savevm(self):
+        for i in range(10):
+            result = self.vm.qmp('human-monitor-command', command_line='savevm')
+            self.assert_qmp(result, 'return', '')
+
+            result = self.vm.qmp('migrate_set_speed', value=9223372036853727232)
+            self.assert_qmp(result, 'return', {})
+
+if __name__ == '__main__':
+    iotests.main()
diff --git a/tests/qemu-iotests/175.out b/tests/qemu-iotests/175.out
new file mode 100644
index 0000000000..8d7e996700
--- /dev/null
+++ b/tests/qemu-iotests/175.out
@@ -0,0 +1,5 @@
+...
+----------------------------------------------------------------------
+Ran 3 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 985b9a6a36..1f4bf03185 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -167,3 +167,4 @@
 172 auto
 173 rw auto
 174 auto
+175 auto quick
-- 
2.11.1

[Qemu-devel] [PATCH 2/4] qmp-cont: invalidate on RUN_STATE_PRELAUNCH

[Vladimir Sementsov-Ogievskiy]

We must invalidate on RUN_STATE_PRELAUNCH too, as it is available
through qmp_system_reset from RUN_STATE_POSTMIGRATE. Otherwise, we will
come to

qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
       Assertion `!(bs->open_flags & 0x0800)' failed.

on the first write after vm start.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qmp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qmp.c b/qmp.c
index dfaabac1a6..e61795d033 100644
--- a/qmp.c
+++ b/qmp.c
@@ -198,7 +198,8 @@ void qmp_cont(Error **errp)
     /* Continuing after completed migration. Images have been inactivated to
      * allow the destination to take control. Need to get control back now. */
     if (runstate_check(RUN_STATE_FINISH_MIGRATE) ||
-        runstate_check(RUN_STATE_POSTMIGRATE))
+        runstate_check(RUN_STATE_POSTMIGRATE) ||
+        runstate_check(RUN_STATE_PRELAUNCH))
     {
         bdrv_invalidate_cache_all(&local_err);
         if (local_err) {
-- 
2.11.1

[Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Vladimir Sementsov-Ogievskiy]

After migration all drives are inactive and savevm will fail with

qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
   Assertion `!(bs->open_flags & 0x0800)' failed.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/snapshot.c   |  3 ++-
 migration/savevm.c | 11 +++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index bf5c2ca5e1..256d06ac9f 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -145,7 +145,8 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
 int bdrv_can_snapshot(BlockDriverState *bs)
 {
     BlockDriver *drv = bs->drv;
-    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
+    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
+        (bs->open_flags & BDRV_O_INACTIVE)) {
         return 0;
     }
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 5ecd264134..75e56d2d07 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2068,6 +2068,17 @@ int save_vmstate(Monitor *mon, const char *name)
     Error *local_err = NULL;
     AioContext *aio_context;
 
+    if (runstate_check(RUN_STATE_FINISH_MIGRATE) ||
+        runstate_check(RUN_STATE_POSTMIGRATE) ||
+        runstate_check(RUN_STATE_PRELAUNCH))
+    {
+        bdrv_invalidate_cache_all(&local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            return -EINVAL;
+        }
+    }
+
     if (!bdrv_all_can_snapshot(&bs)) {
         monitor_printf(mon, "Device '%s' is writable but does not "
                        "support snapshots.\n", bdrv_get_device_name(bs));
-- 
2.11.1

[Qemu-devel] [PATCH 4/4] migration: fix use-after-free of to_dst_file

[Vladimir Sementsov-Ogievskiy]

hmp_savevm calls qemu_savevm_state(f), which sets to_dst_file=f in
global migration state. Then hmp_savevm closes f (g_free called).

Next access to to_dst_file in migration state (for example,
qmp_migrate_set_speed) will use it after it was freed.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 migration/savevm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 75e56d2d07..fcb8fd8acd 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1276,6 +1276,11 @@ done:
         status = MIGRATION_STATUS_COMPLETED;
     }
     migrate_set_state(&ms->state, MIGRATION_STATUS_SETUP, status);
+
+    /* f is outer parameter, it should not stay in global migration state after
+     * this function finished */
+    ms->to_dst_file = NULL;
+
     return ret;
 }
 
-- 
2.11.1

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Denis V. Lunev]

On 02/25/2017 10:31 PM, Vladimir Sementsov-Ogievskiy wrote:
> After migration all drives are inactive and savevm will fail with
>
> qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
>    Assertion `!(bs->open_flags & 0x0800)' failed.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/snapshot.c   |  3 ++-
>  migration/savevm.c | 11 +++++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/block/snapshot.c b/block/snapshot.c
> index bf5c2ca5e1..256d06ac9f 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -145,7 +145,8 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
>  int bdrv_can_snapshot(BlockDriverState *bs)
>  {
>      BlockDriver *drv = bs->drv;
> -    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
> +    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
> +        (bs->open_flags & BDRV_O_INACTIVE)) {
>          return 0;
>      }
>  
at my opinion we do not need this hunk. It will result in a wrong
thing.


> diff --git a/migration/savevm.c b/migration/savevm.c
> index 5ecd264134..75e56d2d07 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2068,6 +2068,17 @@ int save_vmstate(Monitor *mon, const char *name)
>      Error *local_err = NULL;
>      AioContext *aio_context;
>  
> +    if (runstate_check(RUN_STATE_FINISH_MIGRATE) ||
> +        runstate_check(RUN_STATE_POSTMIGRATE) ||
> +        runstate_check(RUN_STATE_PRELAUNCH))
> +    {
> +        bdrv_invalidate_cache_all(&local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +            return -EINVAL;
> +        }
> +    }
> +
>      if (!bdrv_all_can_snapshot(&bs)) {
>          monitor_printf(mon, "Device '%s' is writable but does not "
>                         "support snapshots.\n", bdrv_get_device_name(bs));

Re: [Qemu-devel] [PATCH 4/4] migration: fix use-after-free of to_dst_file

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> hmp_savevm calls qemu_savevm_state(f), which sets to_dst_file=f in
> global migration state. Then hmp_savevm closes f (g_free called).
> 
> Next access to to_dst_file in migration state (for example,
> qmp_migrate_set_speed) will use it after it was freed.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/savevm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 75e56d2d07..fcb8fd8acd 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1276,6 +1276,11 @@ done:
>          status = MIGRATION_STATUS_COMPLETED;
>      }
>      migrate_set_state(&ms->state, MIGRATION_STATUS_SETUP, status);
> +
> +    /* f is outer parameter, it should not stay in global migration state after
> +     * this function finished */
> +    ms->to_dst_file = NULL;
> +
>      return ret;
>  }
>  
> -- 
> 2.11.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 4/4] migration: fix use-after-free of to_dst_file

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> hmp_savevm calls qemu_savevm_state(f), which sets to_dst_file=f in
> global migration state. Then hmp_savevm closes f (g_free called).
> 
> Next access to to_dst_file in migration state (for example,
> qmp_migrate_set_speed) will use it after it was freed.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Queued just this one.

> ---
>  migration/savevm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 75e56d2d07..fcb8fd8acd 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1276,6 +1276,11 @@ done:
>          status = MIGRATION_STATUS_COMPLETED;
>      }
>      migrate_set_state(&ms->state, MIGRATION_STATUS_SETUP, status);
> +
> +    /* f is outer parameter, it should not stay in global migration state after
> +     * this function finished */
> +    ms->to_dst_file = NULL;
> +
>      return ret;
>  }
>  
> -- 
> 2.11.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/4] some migration bugs

[Denis V. Lunev]

On 02/25/2017 10:31 PM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here are some migration related bugs, two about INACTIVE bdses and one
> use-after-free.
>
> I'm absolutely not sure, that these bugs should be fixed like I'm fixing,
> but problem definitely exists.
>
> Reset in stopped state is strange case, may be such usage should be
> restricted.
> About INACTIVE - looks like it should be a separate run-state, not only
> bdrv-flag.
> Situation with migration state, which is global, but is set/reset/changed
> in not controlled manner is not very good too..
>
> Vladimir Sementsov-Ogievskiy (4):
>   iotests: add migration corner cases test
>   qmp-cont: invalidate on RUN_STATE_PRELAUNCH
>   savevm: fix savevm after migration
>   migration: fix use-after-free of to_dst_file
>
>  block/snapshot.c           |  3 +-
>  migration/savevm.c         | 16 +++++++++++
>  qmp.c                      |  3 +-
>  tests/qemu-iotests/175     | 71 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/175.out |  5 ++++
>  tests/qemu-iotests/group   |  1 +
>  6 files changed, 97 insertions(+), 2 deletions(-)
>  create mode 100644 tests/qemu-iotests/175
>  create mode 100644 tests/qemu-iotests/175.out
>
guys, what about patches 1-3?

Den

Re: [Qemu-devel] [PATCH 1/4] iotests: add migration corner cases test

[Fam Zheng]

On Sat, 02/25 22:31, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/175     | 71 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/175.out |  5 ++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 77 insertions(+)
>  create mode 100644 tests/qemu-iotests/175
>  create mode 100644 tests/qemu-iotests/175.out
> 
> diff --git a/tests/qemu-iotests/175 b/tests/qemu-iotests/175
> new file mode 100644
> index 0000000000..ef86c70db5
> --- /dev/null
> +++ b/tests/qemu-iotests/175
> @@ -0,0 +1,71 @@
> +#!/usr/bin/env python
> +#
> +# Test migration corner-cases
> +#
> +# Copyright (C) Vladimir Sementsov-Ogievskiy 2017
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import os
> +import iotests
> +import time
> +from iotests import qemu_img
> +
> +disk = os.path.join(iotests.test_dir, 'disk')
> +
> +class TestMigrationCornerCases(iotests.QMPTestCase):
> +    def setUp(self):
> +        qemu_img('create', '-f', iotests.imgfmt, disk, '10M')
> +        self.vm = iotests.VM().add_drive(disk)
> +        self.vm.launch()
> +
> +    def tearDown(self):
> +        self.vm.shutdown()
> +        os.remove(disk)
> +
> +    def test_migrate_reset_cont_write(self):
> +        result = self.vm.qmp('migrate', uri='exec:cat>/dev/null')
> +        self.assert_qmp(result, 'return', {})
> +        time.sleep(4)

Should you query the migration status instead of blink sleep?

> +
> +        result = self.vm.qmp('human-monitor-command',
> +                             command_line='system_reset')
> +        self.assert_qmp(result, 'return', '')
> +
> +        result = self.vm.qmp('cont')
> +        self.assert_qmp(result, 'return', {})
> +
> +        result = self.vm.qmp('human-monitor-command',
> +                             command_line='qemu-io drive0 "write 0 512"')
> +        self.assert_qmp(result, 'return', '')
> +
> +    def test_migrate_savevm(self):
> +        result = self.vm.qmp('migrate', uri='exec:cat>/dev/null')
> +        self.assert_qmp(result, 'return', {})
> +        time.sleep(4)

Ditto.

> +
> +        result = self.vm.qmp('human-monitor-command', command_line='savevm')
> +        self.assert_qmp(result, 'return', '')
> +
> +    def test_savevm_set_speed_savevm(self):
> +        for i in range(10):
> +            result = self.vm.qmp('human-monitor-command', command_line='savevm')
> +            self.assert_qmp(result, 'return', '')
> +
> +            result = self.vm.qmp('migrate_set_speed', value=9223372036853727232)
> +            self.assert_qmp(result, 'return', {})
> +
> +if __name__ == '__main__':
> +    iotests.main()
> diff --git a/tests/qemu-iotests/175.out b/tests/qemu-iotests/175.out
> new file mode 100644
> index 0000000000..8d7e996700
> --- /dev/null
> +++ b/tests/qemu-iotests/175.out
> @@ -0,0 +1,5 @@
> +...
> +----------------------------------------------------------------------
> +Ran 3 tests
> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 985b9a6a36..1f4bf03185 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -167,3 +167,4 @@
>  172 auto
>  173 rw auto
>  174 auto
> +175 auto quick

Not sure it still qualifies as "quick" given the two "time.sleep(4)", but more
importantly I wonder if the sleep should be improved.

Fam

Re: [Qemu-devel] [PATCH 2/4] qmp-cont: invalidate on RUN_STATE_PRELAUNCH

[Fam Zheng]

On Sat, 02/25 22:31, Vladimir Sementsov-Ogievskiy wrote:
> We must invalidate on RUN_STATE_PRELAUNCH too, as it is available
> through qmp_system_reset from RUN_STATE_POSTMIGRATE. Otherwise, we will
> come to
> 
> qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
>        Assertion `!(bs->open_flags & 0x0800)' failed.
> 
> on the first write after vm start.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qmp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/qmp.c b/qmp.c
> index dfaabac1a6..e61795d033 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -198,7 +198,8 @@ void qmp_cont(Error **errp)
>      /* Continuing after completed migration. Images have been inactivated to
>       * allow the destination to take control. Need to get control back now. */
>      if (runstate_check(RUN_STATE_FINISH_MIGRATE) ||
> -        runstate_check(RUN_STATE_POSTMIGRATE))
> +        runstate_check(RUN_STATE_POSTMIGRATE) ||
> +        runstate_check(RUN_STATE_PRELAUNCH))
>      {
>          bdrv_invalidate_cache_all(&local_err);
>          if (local_err) {
> -- 
> 2.11.1
> 

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

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Kevin Wolf]

Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
> After migration all drives are inactive and savevm will fail with
> 
> qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
>    Assertion `!(bs->open_flags & 0x0800)' failed.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

What's the exact state you're in? I tried to reproduce this, but just
doing a live migration and then savevm on the destination works fine for
me.

Hm... Or do you mean on the source? In that case, I think the operation
must fail, but of course more gracefully than now.

Actually, the question that you're asking implicitly here is how the
source qemu process should be "reactivated" after a failed migration.
Currently, as far as I know, this is only with issuing a "cont" command.
It might make sense to provide a way to get control without resuming the
VM, but I doubt that adding automatic resume to every QMP command is the
right way to achieve it.

Dave, Juan, what do you think?

> diff --git a/block/snapshot.c b/block/snapshot.c
> index bf5c2ca5e1..256d06ac9f 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -145,7 +145,8 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
>  int bdrv_can_snapshot(BlockDriverState *bs)
>  {
>      BlockDriver *drv = bs->drv;
> -    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
> +    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
> +        (bs->open_flags & BDRV_O_INACTIVE)) {
>          return 0;
>      }

I wasn't sure if this doesn't disable too much, but it seems it only
makes 'info snapshots' turn up empty, which might not be nice, but maybe
tolerable.

At least it should definitely fix the assertion.

> diff --git a/migration/savevm.c b/migration/savevm.c
> index 5ecd264134..75e56d2d07 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2068,6 +2068,17 @@ int save_vmstate(Monitor *mon, const char *name)
>      Error *local_err = NULL;
>      AioContext *aio_context;
>  
> +    if (runstate_check(RUN_STATE_FINISH_MIGRATE) ||
> +        runstate_check(RUN_STATE_POSTMIGRATE) ||
> +        runstate_check(RUN_STATE_PRELAUNCH))
> +    {
> +        bdrv_invalidate_cache_all(&local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +            return -EINVAL;
> +        }
> +    }
> +

This hunk can't go in before the more general question of implicitly or
explicitly regaining control after a failed migration is answered.

Kevin

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Vladimir Sementsov-Ogievskiy]

07.03.2017 12:53, Kevin Wolf wrote:
> Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> After migration all drives are inactive and savevm will fail with
>>
>> qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
>>     Assertion `!(bs->open_flags & 0x0800)' failed.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> What's the exact state you're in? I tried to reproduce this, but just
> doing a live migration and then savevm on the destination works fine for
> me.
>
> Hm... Or do you mean on the source? In that case, I think the operation
> must fail, but of course more gracefully than now.

Yes, I mean on the source. It may not be migration for "mirgration", but 
for dumping state to file. In that case it seems not wrong to make a 
snapshot on source.

>
> Actually, the question that you're asking implicitly here is how the
> source qemu process should be "reactivated" after a failed migration.
> Currently, as far as I know, this is only with issuing a "cont" command.
> It might make sense to provide a way to get control without resuming the
> VM, but I doubt that adding automatic resume to every QMP command is the
> right way to achieve it.
>
> Dave, Juan, what do you think?
>
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index bf5c2ca5e1..256d06ac9f 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -145,7 +145,8 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
>>   int bdrv_can_snapshot(BlockDriverState *bs)
>>   {
>>       BlockDriver *drv = bs->drv;
>> -    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
>> +    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
>> +        (bs->open_flags & BDRV_O_INACTIVE)) {
>>           return 0;
>>       }
> I wasn't sure if this doesn't disable too much, but it seems it only
> makes 'info snapshots' turn up empty, which might not be nice, but maybe
> tolerable.
>
> At least it should definitely fix the assertion.
>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 5ecd264134..75e56d2d07 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2068,6 +2068,17 @@ int save_vmstate(Monitor *mon, const char *name)
>>       Error *local_err = NULL;
>>       AioContext *aio_context;
>>   
>> +    if (runstate_check(RUN_STATE_FINISH_MIGRATE) ||
>> +        runstate_check(RUN_STATE_POSTMIGRATE) ||
>> +        runstate_check(RUN_STATE_PRELAUNCH))
>> +    {
>> +        bdrv_invalidate_cache_all(&local_err);
>> +        if (local_err) {
>> +            error_report_err(local_err);
>> +            return -EINVAL;
>> +        }
>> +    }
>> +
> This hunk can't go in before the more general question of implicitly or
> explicitly regaining control after a failed migration is answered.
>
> Kevin


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/4] qmp-cont: invalidate on RUN_STATE_PRELAUNCH

[Kevin Wolf]

Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
> We must invalidate on RUN_STATE_PRELAUNCH too, as it is available
> through qmp_system_reset from RUN_STATE_POSTMIGRATE. Otherwise, we will
> come to
> 
> qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
>        Assertion `!(bs->open_flags & 0x0800)' failed.
> 
> on the first write after vm start.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Wouldn't it make more sense to invalidate in qmp_system_reset() where
the migration states are left?

Or maybe BDRV_O_INACTIVE could even be tied directly to runstates? Not
sure how realistic this one is, but if we start adding invalidate_cache
calls all over the place, maybe that's a sign that we need to look for a
more central place.

Kevin

>  qmp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/qmp.c b/qmp.c
> index dfaabac1a6..e61795d033 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -198,7 +198,8 @@ void qmp_cont(Error **errp)
>      /* Continuing after completed migration. Images have been inactivated to
>       * allow the destination to take control. Need to get control back now. */
>      if (runstate_check(RUN_STATE_FINISH_MIGRATE) ||
> -        runstate_check(RUN_STATE_POSTMIGRATE))
> +        runstate_check(RUN_STATE_POSTMIGRATE) ||
> +        runstate_check(RUN_STATE_PRELAUNCH))
>      {
>          bdrv_invalidate_cache_all(&local_err);
>          if (local_err) {
> -- 
> 2.11.1
> 

Re: [Qemu-devel] [PATCH 2/4] qmp-cont: invalidate on RUN_STATE_PRELAUNCH

[Vladimir Sementsov-Ogievskiy]

07.03.2017 13:02, Kevin Wolf wrote:
> Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> We must invalidate on RUN_STATE_PRELAUNCH too, as it is available
>> through qmp_system_reset from RUN_STATE_POSTMIGRATE. Otherwise, we will
>> come to
>>
>> qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
>>         Assertion `!(bs->open_flags & 0x0800)' failed.
>>
>> on the first write after vm start.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Wouldn't it make more sense to invalidate in qmp_system_reset() where
> the migration states are left?
>
> Or maybe BDRV_O_INACTIVE could even be tied directly to runstates? Not
> sure how realistic this one is, but if we start adding invalidate_cache
> calls all over the place, maybe that's a sign that we need to look for a
> more central place.

I've proposed it in cover letter) These bugs and my fixes are just show 
that something should be rethought.. I don't claim that these fixes are 
true way, they are just the simplest.

> Kevin
>
>>   qmp.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/qmp.c b/qmp.c
>> index dfaabac1a6..e61795d033 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -198,7 +198,8 @@ void qmp_cont(Error **errp)
>>       /* Continuing after completed migration. Images have been inactivated to
>>        * allow the destination to take control. Need to get control back now. */
>>       if (runstate_check(RUN_STATE_FINISH_MIGRATE) ||
>> -        runstate_check(RUN_STATE_POSTMIGRATE))
>> +        runstate_check(RUN_STATE_POSTMIGRATE) ||
>> +        runstate_check(RUN_STATE_PRELAUNCH))
>>       {
>>           bdrv_invalidate_cache_all(&local_err);
>>           if (local_err) {
>> -- 
>> 2.11.1
>>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/4] qmp-cont: invalidate on RUN_STATE_PRELAUNCH

[Kevin Wolf]

Am 07.03.2017 um 11:11 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 07.03.2017 13:02, Kevin Wolf wrote:
> >Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>We must invalidate on RUN_STATE_PRELAUNCH too, as it is available
> >>through qmp_system_reset from RUN_STATE_POSTMIGRATE. Otherwise, we will
> >>come to
> >>
> >>qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
> >>        Assertion `!(bs->open_flags & 0x0800)' failed.
> >>
> >>on the first write after vm start.
> >>
> >>Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >Wouldn't it make more sense to invalidate in qmp_system_reset() where
> >the migration states are left?
> >
> >Or maybe BDRV_O_INACTIVE could even be tied directly to runstates? Not
> >sure how realistic this one is, but if we start adding invalidate_cache
> >calls all over the place, maybe that's a sign that we need to look for a
> >more central place.
> 
> I've proposed it in cover letter) These bugs and my fixes are just
> show that something should be rethought.. I don't claim that these
> fixes are true way, they are just the simplest.

Ah, sorry, I read the cover letter a while ago, but not again before
reading the patches now. You're right that it's something that needs to
be addressed in some way. Your patches may be simple, but I think there
is no hope to ever get it completely correct with that approach because
there are so many cases (basically each QMP command is one case).

The cover letter seems to propose that INACTIVE could be a runstate. I
don't think that's quite right, because we already have many runstates
that would qualify as inactive in this sense, but that still need to be
separate. But possibly we can have a mapping from runstates to the
inactive flag.

One (implementation) problem with directly coupling inactive with
runstates is that runstate_set() can't fail at the moment, but
bdrv_inactivate/invalidate_cache() can.

Kevin

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Kevin Wolf]

Am 07.03.2017 um 10:59 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 07.03.2017 12:53, Kevin Wolf wrote:
> >Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>After migration all drives are inactive and savevm will fail with
> >>
> >>qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
> >>    Assertion `!(bs->open_flags & 0x0800)' failed.
> >>
> >>Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >What's the exact state you're in? I tried to reproduce this, but just
> >doing a live migration and then savevm on the destination works fine for
> >me.
> >
> >Hm... Or do you mean on the source? In that case, I think the operation
> >must fail, but of course more gracefully than now.
> 
> Yes, I mean on the source. It may not be migration for "mirgration",
> but for dumping state to file. In that case it seems not wrong to
> make a snapshot on source.

Yes, resuming on the source is definitely valid. I'm only questioning if
'savevm' (and by extension, any other command that modifies the VM or
its images) should automagically regain control of the VM, or whether
that should be more explicit.

Kevin

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) wrote:
> Am 07.03.2017 um 10:59 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > 07.03.2017 12:53, Kevin Wolf wrote:
> > >Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > >>After migration all drives are inactive and savevm will fail with
> > >>
> > >>qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
> > >>    Assertion `!(bs->open_flags & 0x0800)' failed.
> > >>
> > >>Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > >What's the exact state you're in? I tried to reproduce this, but just
> > >doing a live migration and then savevm on the destination works fine for
> > >me.
> > >
> > >Hm... Or do you mean on the source? In that case, I think the operation
> > >must fail, but of course more gracefully than now.
> > 
> > Yes, I mean on the source. It may not be migration for "mirgration",
> > but for dumping state to file. In that case it seems not wrong to
> > make a snapshot on source.
> 
> Yes, resuming on the source is definitely valid. I'm only questioning if
> 'savevm' (and by extension, any other command that modifies the VM or
> its images) should automagically regain control of the VM, or whether
> that should be more explicit.

How many things other than 'cont' and 'savevm' are there?

We should definitely fix it so  a savevm there doesn't assert,
but I'm also unsure if it's worth a separate explicit command.

Dave

> Kevin
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/4] iotests: add migration corner cases test

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

If I understand this correctly, these are tests that currently fail and
are fixed by the following patches;  in which case it needs reordering
to keep stuff bisectable.

Dave

> ---
>  tests/qemu-iotests/175     | 71 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/175.out |  5 ++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 77 insertions(+)
>  create mode 100644 tests/qemu-iotests/175
>  create mode 100644 tests/qemu-iotests/175.out
> 
> diff --git a/tests/qemu-iotests/175 b/tests/qemu-iotests/175
> new file mode 100644
> index 0000000000..ef86c70db5
> --- /dev/null
> +++ b/tests/qemu-iotests/175
> @@ -0,0 +1,71 @@
> +#!/usr/bin/env python
> +#
> +# Test migration corner-cases
> +#
> +# Copyright (C) Vladimir Sementsov-Ogievskiy 2017
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import os
> +import iotests
> +import time
> +from iotests import qemu_img
> +
> +disk = os.path.join(iotests.test_dir, 'disk')
> +
> +class TestMigrationCornerCases(iotests.QMPTestCase):
> +    def setUp(self):
> +        qemu_img('create', '-f', iotests.imgfmt, disk, '10M')
> +        self.vm = iotests.VM().add_drive(disk)
> +        self.vm.launch()
> +
> +    def tearDown(self):
> +        self.vm.shutdown()
> +        os.remove(disk)
> +
> +    def test_migrate_reset_cont_write(self):
> +        result = self.vm.qmp('migrate', uri='exec:cat>/dev/null')
> +        self.assert_qmp(result, 'return', {})
> +        time.sleep(4)
> +
> +        result = self.vm.qmp('human-monitor-command',
> +                             command_line='system_reset')
> +        self.assert_qmp(result, 'return', '')
> +
> +        result = self.vm.qmp('cont')
> +        self.assert_qmp(result, 'return', {})
> +
> +        result = self.vm.qmp('human-monitor-command',
> +                             command_line='qemu-io drive0 "write 0 512"')
> +        self.assert_qmp(result, 'return', '')
> +
> +    def test_migrate_savevm(self):
> +        result = self.vm.qmp('migrate', uri='exec:cat>/dev/null')
> +        self.assert_qmp(result, 'return', {})
> +        time.sleep(4)
> +
> +        result = self.vm.qmp('human-monitor-command', command_line='savevm')
> +        self.assert_qmp(result, 'return', '')
> +
> +    def test_savevm_set_speed_savevm(self):
> +        for i in range(10):
> +            result = self.vm.qmp('human-monitor-command', command_line='savevm')
> +            self.assert_qmp(result, 'return', '')
> +
> +            result = self.vm.qmp('migrate_set_speed', value=9223372036853727232)
> +            self.assert_qmp(result, 'return', {})
> +
> +if __name__ == '__main__':
> +    iotests.main()
> diff --git a/tests/qemu-iotests/175.out b/tests/qemu-iotests/175.out
> new file mode 100644
> index 0000000000..8d7e996700
> --- /dev/null
> +++ b/tests/qemu-iotests/175.out
> @@ -0,0 +1,5 @@
> +...
> +----------------------------------------------------------------------
> +Ran 3 tests
> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 985b9a6a36..1f4bf03185 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -167,3 +167,4 @@
>  172 auto
>  173 rw auto
>  174 auto
> +175 auto quick
> -- 
> 2.11.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] ping Re: [PATCH 0/4] some migration bugs

[Vladimir Sementsov-Ogievskiy]

06.03.2017 17:23, Denis V. Lunev wrote:
> On 02/25/2017 10:31 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here are some migration related bugs, two about INACTIVE bdses and one
>> use-after-free.
>>
>> I'm absolutely not sure, that these bugs should be fixed like I'm fixing,
>> but problem definitely exists.
>>
>> Reset in stopped state is strange case, may be such usage should be
>> restricted.
>> About INACTIVE - looks like it should be a separate run-state, not only
>> bdrv-flag.
>> Situation with migration state, which is global, but is set/reset/changed
>> in not controlled manner is not very good too..
>>
>> Vladimir Sementsov-Ogievskiy (4):
>>    iotests: add migration corner cases test
>>    qmp-cont: invalidate on RUN_STATE_PRELAUNCH
>>    savevm: fix savevm after migration
>>    migration: fix use-after-free of to_dst_file
>>
>>   block/snapshot.c           |  3 +-
>>   migration/savevm.c         | 16 +++++++++++
>>   qmp.c                      |  3 +-
>>   tests/qemu-iotests/175     | 71 ++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/175.out |  5 ++++
>>   tests/qemu-iotests/group   |  1 +
>>   6 files changed, 97 insertions(+), 2 deletions(-)
>>   create mode 100644 tests/qemu-iotests/175
>>   create mode 100644 tests/qemu-iotests/175.out
>>
> guys, what about patches 1-3?
>
> Den

At least, with 2 and 3, fixing an assert? Do someone going to work with 
adding a runstate?


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) wrote:
> Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > After migration all drives are inactive and savevm will fail with
> > 
> > qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
> >    Assertion `!(bs->open_flags & 0x0800)' failed.
> > 
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> What's the exact state you're in? I tried to reproduce this, but just
> doing a live migration and then savevm on the destination works fine for
> me.
> 
> Hm... Or do you mean on the source? In that case, I think the operation
> must fail, but of course more gracefully than now.
> 
> Actually, the question that you're asking implicitly here is how the
> source qemu process should be "reactivated" after a failed migration.
> Currently, as far as I know, this is only with issuing a "cont" command.
> It might make sense to provide a way to get control without resuming the
> VM, but I doubt that adding automatic resume to every QMP command is the
> right way to achieve it.
> 
> Dave, Juan, what do you think?

I'd only ever really thought of 'cont' or retrying the migration.
However, it does make sense to me that you might want to do a savevm instead;
if you can't migrate then perhaps a savevm is the best you can do before
your machine dies.  Are there any other things that should be allowed?

We would want to be careful not to accidentally reactivate the disks on the source
after what was actually a succesful migration.

As for the actual patch contents, I'd leave that to you to say if it's
OK from the block side of things.

Dave

> > diff --git a/block/snapshot.c b/block/snapshot.c
> > index bf5c2ca5e1..256d06ac9f 100644
> > --- a/block/snapshot.c
> > +++ b/block/snapshot.c
> > @@ -145,7 +145,8 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
> >  int bdrv_can_snapshot(BlockDriverState *bs)
> >  {
> >      BlockDriver *drv = bs->drv;
> > -    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
> > +    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
> > +        (bs->open_flags & BDRV_O_INACTIVE)) {
> >          return 0;
> >      }
> 
> I wasn't sure if this doesn't disable too much, but it seems it only
> makes 'info snapshots' turn up empty, which might not be nice, but maybe
> tolerable.
> 
> At least it should definitely fix the assertion.

Did Denis have some concerns about this chunk?

> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 5ecd264134..75e56d2d07 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2068,6 +2068,17 @@ int save_vmstate(Monitor *mon, const char *name)
> >      Error *local_err = NULL;
> >      AioContext *aio_context;
> >  
> > +    if (runstate_check(RUN_STATE_FINISH_MIGRATE) ||
> > +        runstate_check(RUN_STATE_POSTMIGRATE) ||
> > +        runstate_check(RUN_STATE_PRELAUNCH))
> > +    {
> > +        bdrv_invalidate_cache_all(&local_err);
> > +        if (local_err) {
> > +            error_report_err(local_err);
> > +            return -EINVAL;
> > +        }
> > +    }
> > +
> 
> This hunk can't go in before the more general question of implicitly or
> explicitly regaining control after a failed migration is answered.
> 
> Kevin
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Kevin Wolf]

Am 28.03.2017 um 12:55 hat Dr. David Alan Gilbert geschrieben:
> * Kevin Wolf (kwolf@redhat.com) wrote:
> > Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > After migration all drives are inactive and savevm will fail with
> > > 
> > > qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
> > >    Assertion `!(bs->open_flags & 0x0800)' failed.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > 
> > What's the exact state you're in? I tried to reproduce this, but just
> > doing a live migration and then savevm on the destination works fine for
> > me.
> > 
> > Hm... Or do you mean on the source? In that case, I think the operation
> > must fail, but of course more gracefully than now.
> > 
> > Actually, the question that you're asking implicitly here is how the
> > source qemu process should be "reactivated" after a failed migration.
> > Currently, as far as I know, this is only with issuing a "cont" command.
> > It might make sense to provide a way to get control without resuming the
> > VM, but I doubt that adding automatic resume to every QMP command is the
> > right way to achieve it.
> > 
> > Dave, Juan, what do you think?
> 
> I'd only ever really thought of 'cont' or retrying the migration.
> However, it does make sense to me that you might want to do a savevm
> instead; if you can't migrate then perhaps a savevm is the best you
> can do before your machine dies.  Are there any other things that
> should be allowed?

I think we need to ask the other way round: Any reason _not_ to allow
certain operations that you can normally perform on a stopped VM?

> We would want to be careful not to accidentally reactivate the disks
> on the source after what was actually a succesful migration.

Yes, that's exactly my concern, even with savevm. That's why I suggested
we could have a 'cont'-like thing that just gets back control of the
images and moves into the normal paused state, but doesn't immediately
resume the actual VM.

Kevin

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) wrote:
> Am 28.03.2017 um 12:55 hat Dr. David Alan Gilbert geschrieben:
> > * Kevin Wolf (kwolf@redhat.com) wrote:
> > > Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > After migration all drives are inactive and savevm will fail with
> > > > 
> > > > qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
> > > >    Assertion `!(bs->open_flags & 0x0800)' failed.
> > > > 
> > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > 
> > > What's the exact state you're in? I tried to reproduce this, but just
> > > doing a live migration and then savevm on the destination works fine for
> > > me.
> > > 
> > > Hm... Or do you mean on the source? In that case, I think the operation
> > > must fail, but of course more gracefully than now.
> > > 
> > > Actually, the question that you're asking implicitly here is how the
> > > source qemu process should be "reactivated" after a failed migration.
> > > Currently, as far as I know, this is only with issuing a "cont" command.
> > > It might make sense to provide a way to get control without resuming the
> > > VM, but I doubt that adding automatic resume to every QMP command is the
> > > right way to achieve it.
> > > 
> > > Dave, Juan, what do you think?
> > 
> > I'd only ever really thought of 'cont' or retrying the migration.
> > However, it does make sense to me that you might want to do a savevm
> > instead; if you can't migrate then perhaps a savevm is the best you
> > can do before your machine dies.  Are there any other things that
> > should be allowed?
> 
> I think we need to ask the other way round: Any reason _not_ to allow
> certain operations that you can normally perform on a stopped VM?
> 
> > We would want to be careful not to accidentally reactivate the disks
> > on the source after what was actually a succesful migration.
> 
> Yes, that's exactly my concern, even with savevm. That's why I suggested
> we could have a 'cont'-like thing that just gets back control of the
> images and moves into the normal paused state, but doesn't immediately
> resume the actual VM.

OK, lets say we had that block-reactivate (for want of a better name),
how would we stop everything asserting if the user tried to do it 
before they'd run block-reactivate?

Dave

> Kevin
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Denis V. Lunev]

On 03/28/2017 01:55 PM, Dr. David Alan Gilbert wrote:
> * Kevin Wolf (kwolf@redhat.com) wrote:
>> Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> After migration all drives are inactive and savevm will fail with
>>>
>>> qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
>>>    Assertion `!(bs->open_flags & 0x0800)' failed.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> What's the exact state you're in? I tried to reproduce this, but just
>> doing a live migration and then savevm on the destination works fine for
>> me.
>>
>> Hm... Or do you mean on the source? In that case, I think the operation
>> must fail, but of course more gracefully than now.
>>
>> Actually, the question that you're asking implicitly here is how the
>> source qemu process should be "reactivated" after a failed migration.
>> Currently, as far as I know, this is only with issuing a "cont" command.
>> It might make sense to provide a way to get control without resuming the
>> VM, but I doubt that adding automatic resume to every QMP command is the
>> right way to achieve it.
>>
>> Dave, Juan, what do you think?
> I'd only ever really thought of 'cont' or retrying the migration.
> However, it does make sense to me that you might want to do a savevm instead;
> if you can't migrate then perhaps a savevm is the best you can do before
> your machine dies.  Are there any other things that should be allowed?
>
> We would want to be careful not to accidentally reactivate the disks on the source
> after what was actually a succesful migration.
>
> As for the actual patch contents, I'd leave that to you to say if it's
> OK from the block side of things.
>
> Dave
>
>>> diff --git a/block/snapshot.c b/block/snapshot.c
>>> index bf5c2ca5e1..256d06ac9f 100644
>>> --- a/block/snapshot.c
>>> +++ b/block/snapshot.c
>>> @@ -145,7 +145,8 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
>>>  int bdrv_can_snapshot(BlockDriverState *bs)
>>>  {
>>>      BlockDriver *drv = bs->drv;
>>> -    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
>>> +    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) ||
>>> +        (bs->open_flags & BDRV_O_INACTIVE)) {
>>>          return 0;
>>>      }
>> I wasn't sure if this doesn't disable too much, but it seems it only
>> makes 'info snapshots' turn up empty, which might not be nice, but maybe
>> tolerable.
>>
>> At least it should definitely fix the assertion.
> Did Denis have some concerns about this chunk?
Yep. I really think that this check is unnecessary and wrong.
Actually all disks are in the INACTIVE state and we will face
the problem later on the actual write. This exact operation
is sane.

Den

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Kevin Wolf]

Am 28.03.2017 um 13:13 hat Dr. David Alan Gilbert geschrieben:
> * Kevin Wolf (kwolf@redhat.com) wrote:
> > Am 28.03.2017 um 12:55 hat Dr. David Alan Gilbert geschrieben:
> > > * Kevin Wolf (kwolf@redhat.com) wrote:
> > > > Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > > After migration all drives are inactive and savevm will fail with
> > > > > 
> > > > > qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
> > > > >    Assertion `!(bs->open_flags & 0x0800)' failed.
> > > > > 
> > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > > 
> > > > What's the exact state you're in? I tried to reproduce this, but just
> > > > doing a live migration and then savevm on the destination works fine for
> > > > me.
> > > > 
> > > > Hm... Or do you mean on the source? In that case, I think the operation
> > > > must fail, but of course more gracefully than now.
> > > > 
> > > > Actually, the question that you're asking implicitly here is how the
> > > > source qemu process should be "reactivated" after a failed migration.
> > > > Currently, as far as I know, this is only with issuing a "cont" command.
> > > > It might make sense to provide a way to get control without resuming the
> > > > VM, but I doubt that adding automatic resume to every QMP command is the
> > > > right way to achieve it.
> > > > 
> > > > Dave, Juan, what do you think?
> > > 
> > > I'd only ever really thought of 'cont' or retrying the migration.
> > > However, it does make sense to me that you might want to do a savevm
> > > instead; if you can't migrate then perhaps a savevm is the best you
> > > can do before your machine dies.  Are there any other things that
> > > should be allowed?
> > 
> > I think we need to ask the other way round: Any reason _not_ to allow
> > certain operations that you can normally perform on a stopped VM?
> > 
> > > We would want to be careful not to accidentally reactivate the disks
> > > on the source after what was actually a succesful migration.
> > 
> > Yes, that's exactly my concern, even with savevm. That's why I suggested
> > we could have a 'cont'-like thing that just gets back control of the
> > images and moves into the normal paused state, but doesn't immediately
> > resume the actual VM.
> 
> OK, lets say we had that block-reactivate (for want of a better name),
> how would we stop everything asserting if the user tried to do it 
> before they'd run block-reactivate?

We would have to add checks to the monitor commands that assume that the
image is activated and error out if it isn't.

Maybe just adding the check to blk_is_available() would be enough, but
we'd have to check carefully whether it covers all cases and causes no
false positives.

By the way, I wouldn't call this 'block-reactivate' because I don't
think this should be a block-specific command. It's a VM lifecycle
command that switches from a postmigrate state (that assumes we have no
control over the VM's resources any more) to a paused state (where we do
have this control). Maybe something like 'migration-abort'.

Kevin

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Vladimir Sementsov-Ogievskiy]

28.03.2017 15:09, Kevin Wolf wrote:
> Am 28.03.2017 um 13:13 hat Dr. David Alan Gilbert geschrieben:
>> * Kevin Wolf (kwolf@redhat.com) wrote:
>>> Am 28.03.2017 um 12:55 hat Dr. David Alan Gilbert geschrieben:
>>>> * Kevin Wolf (kwolf@redhat.com) wrote:
>>>>> Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>> After migration all drives are inactive and savevm will fail with
>>>>>>
>>>>>> qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
>>>>>>     Assertion `!(bs->open_flags & 0x0800)' failed.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> What's the exact state you're in? I tried to reproduce this, but just
>>>>> doing a live migration and then savevm on the destination works fine for
>>>>> me.
>>>>>
>>>>> Hm... Or do you mean on the source? In that case, I think the operation
>>>>> must fail, but of course more gracefully than now.
>>>>>
>>>>> Actually, the question that you're asking implicitly here is how the
>>>>> source qemu process should be "reactivated" after a failed migration.
>>>>> Currently, as far as I know, this is only with issuing a "cont" command.
>>>>> It might make sense to provide a way to get control without resuming the
>>>>> VM, but I doubt that adding automatic resume to every QMP command is the
>>>>> right way to achieve it.
>>>>>
>>>>> Dave, Juan, what do you think?
>>>> I'd only ever really thought of 'cont' or retrying the migration.
>>>> However, it does make sense to me that you might want to do a savevm
>>>> instead; if you can't migrate then perhaps a savevm is the best you
>>>> can do before your machine dies.  Are there any other things that
>>>> should be allowed?
>>> I think we need to ask the other way round: Any reason _not_ to allow
>>> certain operations that you can normally perform on a stopped VM?
>>>
>>>> We would want to be careful not to accidentally reactivate the disks
>>>> on the source after what was actually a succesful migration.
>>> Yes, that's exactly my concern, even with savevm. That's why I suggested
>>> we could have a 'cont'-like thing that just gets back control of the
>>> images and moves into the normal paused state, but doesn't immediately
>>> resume the actual VM.
>> OK, lets say we had that block-reactivate (for want of a better name),
>> how would we stop everything asserting if the user tried to do it
>> before they'd run block-reactivate?
> We would have to add checks to the monitor commands that assume that the
> image is activated and error out if it isn't.
>
> Maybe just adding the check to blk_is_available() would be enough, but
> we'd have to check carefully whether it covers all cases and causes no
> false positives.
>
> By the way, I wouldn't call this 'block-reactivate' because I don't
> think this should be a block-specific command. It's a VM lifecycle
> command that switches from a postmigrate state (that assumes we have no
> control over the VM's resources any more) to a paused state (where we do
> have this control). Maybe something like 'migration-abort'.

'abort' is not very good too I think. migration is completed, nothing to 
abort.. (may be successful migration to file for suspend, some kind of 
vm cloning, etc)

>
> Kevin


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Paolo Bonzini]

On 28/03/2017 15:16, Vladimir Sementsov-Ogievskiy wrote:
> 28.03.2017 15:09, Kevin Wolf wrote:
>> Am 28.03.2017 um 13:13 hat Dr. David Alan Gilbert geschrieben:
>>> * Kevin Wolf (kwolf@redhat.com) wrote:
>>>> Am 28.03.2017 um 12:55 hat Dr. David Alan Gilbert geschrieben:
>>>>> * Kevin Wolf (kwolf@redhat.com) wrote:
>>>>>> Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>>> After migration all drives are inactive and savevm will fail with
>>>>>>>
>>>>>>> qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
>>>>>>>     Assertion `!(bs->open_flags & 0x0800)' failed.
>>>>>>>
>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>>>>>> <vsementsov@virtuozzo.com>
>>>>>> What's the exact state you're in? I tried to reproduce this, but just
>>>>>> doing a live migration and then savevm on the destination works
>>>>>> fine for
>>>>>> me.
>>>>>>
>>>>>> Hm... Or do you mean on the source? In that case, I think the
>>>>>> operation
>>>>>> must fail, but of course more gracefully than now.
>>>>>>
>>>>>> Actually, the question that you're asking implicitly here is how the
>>>>>> source qemu process should be "reactivated" after a failed migration.
>>>>>> Currently, as far as I know, this is only with issuing a "cont"
>>>>>> command.
>>>>>> It might make sense to provide a way to get control without
>>>>>> resuming the
>>>>>> VM, but I doubt that adding automatic resume to every QMP command
>>>>>> is the
>>>>>> right way to achieve it.
>>>>>>
>>>>>> Dave, Juan, what do you think?
>>>>> I'd only ever really thought of 'cont' or retrying the migration.
>>>>> However, it does make sense to me that you might want to do a savevm
>>>>> instead; if you can't migrate then perhaps a savevm is the best you
>>>>> can do before your machine dies.  Are there any other things that
>>>>> should be allowed?
>>>> I think we need to ask the other way round: Any reason _not_ to allow
>>>> certain operations that you can normally perform on a stopped VM?
>>>>
>>>>> We would want to be careful not to accidentally reactivate the disks
>>>>> on the source after what was actually a succesful migration.
>>>> Yes, that's exactly my concern, even with savevm. That's why I
>>>> suggested
>>>> we could have a 'cont'-like thing that just gets back control of the
>>>> images and moves into the normal paused state, but doesn't immediately
>>>> resume the actual VM.
>>> OK, lets say we had that block-reactivate (for want of a better name),
>>> how would we stop everything asserting if the user tried to do it
>>> before they'd run block-reactivate?
>> We would have to add checks to the monitor commands that assume that the
>> image is activated and error out if it isn't.
>>
>> Maybe just adding the check to blk_is_available() would be enough, but
>> we'd have to check carefully whether it covers all cases and causes no
>> false positives.
>>
>> By the way, I wouldn't call this 'block-reactivate' because I don't
>> think this should be a block-specific command. It's a VM lifecycle
>> command that switches from a postmigrate state (that assumes we have no
>> control over the VM's resources any more) to a paused state (where we do
>> have this control). Maybe something like 'migration-abort'.
> 
> 'abort' is not very good too I think. migration is completed, nothing to
> abort.. (may be successful migration to file for suspend, some kind of
> vm cloning, etc)

There is already migrate_cancel.  Does it make sense to make it
reactivate fds if migration is completed?

Paolo

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 28/03/2017 15:16, Vladimir Sementsov-Ogievskiy wrote:
> > 28.03.2017 15:09, Kevin Wolf wrote:
> >> Am 28.03.2017 um 13:13 hat Dr. David Alan Gilbert geschrieben:
> >>> * Kevin Wolf (kwolf@redhat.com) wrote:
> >>>> Am 28.03.2017 um 12:55 hat Dr. David Alan Gilbert geschrieben:
> >>>>> * Kevin Wolf (kwolf@redhat.com) wrote:
> >>>>>> Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>>>>>> After migration all drives are inactive and savevm will fail with
> >>>>>>>
> >>>>>>> qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
> >>>>>>>     Assertion `!(bs->open_flags & 0x0800)' failed.
> >>>>>>>
> >>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
> >>>>>>> <vsementsov@virtuozzo.com>
> >>>>>> What's the exact state you're in? I tried to reproduce this, but just
> >>>>>> doing a live migration and then savevm on the destination works
> >>>>>> fine for
> >>>>>> me.
> >>>>>>
> >>>>>> Hm... Or do you mean on the source? In that case, I think the
> >>>>>> operation
> >>>>>> must fail, but of course more gracefully than now.
> >>>>>>
> >>>>>> Actually, the question that you're asking implicitly here is how the
> >>>>>> source qemu process should be "reactivated" after a failed migration.
> >>>>>> Currently, as far as I know, this is only with issuing a "cont"
> >>>>>> command.
> >>>>>> It might make sense to provide a way to get control without
> >>>>>> resuming the
> >>>>>> VM, but I doubt that adding automatic resume to every QMP command
> >>>>>> is the
> >>>>>> right way to achieve it.
> >>>>>>
> >>>>>> Dave, Juan, what do you think?
> >>>>> I'd only ever really thought of 'cont' or retrying the migration.
> >>>>> However, it does make sense to me that you might want to do a savevm
> >>>>> instead; if you can't migrate then perhaps a savevm is the best you
> >>>>> can do before your machine dies.  Are there any other things that
> >>>>> should be allowed?
> >>>> I think we need to ask the other way round: Any reason _not_ to allow
> >>>> certain operations that you can normally perform on a stopped VM?
> >>>>
> >>>>> We would want to be careful not to accidentally reactivate the disks
> >>>>> on the source after what was actually a succesful migration.
> >>>> Yes, that's exactly my concern, even with savevm. That's why I
> >>>> suggested
> >>>> we could have a 'cont'-like thing that just gets back control of the
> >>>> images and moves into the normal paused state, but doesn't immediately
> >>>> resume the actual VM.
> >>> OK, lets say we had that block-reactivate (for want of a better name),
> >>> how would we stop everything asserting if the user tried to do it
> >>> before they'd run block-reactivate?
> >> We would have to add checks to the monitor commands that assume that the
> >> image is activated and error out if it isn't.
> >>
> >> Maybe just adding the check to blk_is_available() would be enough, but
> >> we'd have to check carefully whether it covers all cases and causes no
> >> false positives.
> >>
> >> By the way, I wouldn't call this 'block-reactivate' because I don't
> >> think this should be a block-specific command. It's a VM lifecycle
> >> command that switches from a postmigrate state (that assumes we have no
> >> control over the VM's resources any more) to a paused state (where we do
> >> have this control). Maybe something like 'migration-abort'.
> > 
> > 'abort' is not very good too I think. migration is completed, nothing to
> > abort.. (may be successful migration to file for suspend, some kind of
> > vm cloning, etc)
> 
> There is already migrate_cancel.  Does it make sense to make it
> reactivate fds if migration is completed?

It's potentially racy to do that.
Imagine if your migration is almost finished and you issue a migrate_cancel,
what happens?
Maybe it cancelled it.
Maybe it just completed in time - and you really better not be accessing
the disks on the source unless you're sure the destination isn't running.

Dave

> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/4] savevm: fix savevm after migration

[Paolo Bonzini]

On 29/03/2017 17:29, Dr. David Alan Gilbert wrote:
>>> 'abort' is not very good too I think. migration is completed, nothing to
>>> abort.. (may be successful migration to file for suspend, some kind of
>>> vm cloning, etc)
>> There is already migrate_cancel.  Does it make sense to make it
>> reactivate fds if migration is completed?
> It's potentially racy to do that.
> Imagine if your migration is almost finished and you issue a migrate_cancel,
> what happens?
> Maybe it cancelled it.
> Maybe it just completed in time - and you really better not be accessing
> the disks on the source unless you're sure the destination isn't running.

If you want to avoid races, you probably have to use -S anyway on the
destination and do more handshaking between source and destination.  But
yes, just to be safe it'd be better to add a new flag (-f for HMP,
'cancel-completed-migration':true for QMP or something like that).

Paolo

[Qemu-devel] ping Re: [PATCH 3/4] savevm: fix savevm after migration

[Vladimir Sementsov-Ogievskiy]

29.03.2017 18:53, Paolo Bonzini wrote:
>
> On 29/03/2017 17:29, Dr. David Alan Gilbert wrote:
>>>> 'abort' is not very good too I think. migration is completed, nothing to
>>>> abort.. (may be successful migration to file for suspend, some kind of
>>>> vm cloning, etc)
>>> There is already migrate_cancel.  Does it make sense to make it
>>> reactivate fds if migration is completed?
>> It's potentially racy to do that.
>> Imagine if your migration is almost finished and you issue a migrate_cancel,
>> what happens?
>> Maybe it cancelled it.
>> Maybe it just completed in time - and you really better not be accessing
>> the disks on the source unless you're sure the destination isn't running.
> If you want to avoid races, you probably have to use -S anyway on the
> destination and do more handshaking between source and destination.  But
> yes, just to be safe it'd be better to add a new flag (-f for HMP,
> 'cancel-completed-migration':true for QMP or something like that).
>
> Paolo

Looks like we haven't final conclusion on this (solve problems, noted in 
patches 2 and 3). Are someone working on this now?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [Qemu-block] [PATCH 2/4] qmp-cont: invalidate on RUN_STATE_PRELAUNCH

[Kashyap Chamarthy]

On Tue, Mar 07, 2017 at 01:11:23PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 07.03.2017 13:02, Kevin Wolf wrote:
> > Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > We must invalidate on RUN_STATE_PRELAUNCH too, as it is available
> > > through qmp_system_reset from RUN_STATE_POSTMIGRATE. Otherwise, we will
> > > come to
> > > 
> > > qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
> > >         Assertion `!(bs->open_flags & 0x0800)' failed.
> > > 
> > > on the first write after vm start.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > Wouldn't it make more sense to invalidate in qmp_system_reset() where
> > the migration states are left?
> > 
> > Or maybe BDRV_O_INACTIVE could even be tied directly to runstates? Not
> > sure how realistic this one is, but if we start adding invalidate_cache
> > calls all over the place, maybe that's a sign that we need to look for a
> > more central place.
> 
> I've proposed it in cover letter) These bugs and my fixes are just show that
> something should be rethought.. I don't claim that these fixes are true way,
> they are just the simplest.

Hi Vladimir,

I wonder if you have a new version of your patch ("qmp-cont: invalidate
on RUN_STATE_PRELAUNCH").

Hailiang Zhang tells me on this the below thread that your patch fixes
the issue:

  http://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg03925.html
  -- [QEMU-2.8] Source QEMU crashes with: "bdrv_co_pwritev: Assertion
  `!(bs->open_flags & BDRV_O_INACTIVE)' failed"

-- 
/kashyap

Re: [Qemu-devel] [Qemu-block] [PATCH 2/4] qmp-cont: invalidate on RUN_STATE_PRELAUNCH

[Vladimir Sementsov-Ogievskiy]

26.04.2017 15:22, Kashyap Chamarthy wrote:
> On Tue, Mar 07, 2017 at 01:11:23PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 07.03.2017 13:02, Kevin Wolf wrote:
>>> Am 25.02.2017 um 20:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> We must invalidate on RUN_STATE_PRELAUNCH too, as it is available
>>>> through qmp_system_reset from RUN_STATE_POSTMIGRATE. Otherwise, we will
>>>> come to
>>>>
>>>> qemu-kvm: block/io.c:1406: bdrv_co_do_pwritev:
>>>>          Assertion `!(bs->open_flags & 0x0800)' failed.
>>>>
>>>> on the first write after vm start.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Wouldn't it make more sense to invalidate in qmp_system_reset() where
>>> the migration states are left?
>>>
>>> Or maybe BDRV_O_INACTIVE could even be tied directly to runstates? Not
>>> sure how realistic this one is, but if we start adding invalidate_cache
>>> calls all over the place, maybe that's a sign that we need to look for a
>>> more central place.
>> I've proposed it in cover letter) These bugs and my fixes are just show that
>> something should be rethought.. I don't claim that these fixes are true way,
>> they are just the simplest.
> Hi Vladimir,
>
> I wonder if you have a new version of your patch ("qmp-cont: invalidate
> on RUN_STATE_PRELAUNCH").
>
> Hailiang Zhang tells me on this the below thread that your patch fixes
> the issue:
>
>    http://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg03925.html
>    -- [QEMU-2.8] Source QEMU crashes with: "bdrv_co_pwritev: Assertion
>    `!(bs->open_flags & BDRV_O_INACTIVE)' failed"
>

No, I haven't new version, discussion is unfinished about true way of 
doing it. This version is ok with the case it fixes.


-- 
Best regards,
Vladimir