Properly quitting qemu immediately after failing migration

Properly quitting qemu immediately after failing migration

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1852 bytes --]

Hi,

In an iotest, I’m trying to quit qemu immediately after a migration has
failed.  Unfortunately, that doesn’t seem to be possible in a clean way:
migrate_fd_cleanup() runs only at some point after the migration state
is already “failed”, so if I just wait for that “failed” state and
immediately quit, some cleanup functions may not have been run yet.

This is a problem with dirty bitmap migration at least, because it
increases the refcount on all block devices that are to be migrated, so
if we don’t call the cleanup function before quitting, the refcount will
stay elevated and bdrv_close_all() will hit an assertion because those
block devices are still around after blk_remove_all_bs() and
blockdev_close_all_bdrv_states().

In practice this particular issue might not be that big of a problem,
because it just means qemu aborts when the user intended to let it quit
anyway.  But on one hand I could imagine that there are other clean-up
paths that should definitely run before qemu quits (although I don’t
know), and on the other, it’s a problem for my test.

I tried working around the problem for my test by waiting on “Unable to
write” appearing on stderr, because that indicates that
migrate_fd_cleanup()’s error_report_err() has been reached.  But on one
hand, that isn’t really nice, and on the other, it doesn’t even work
when the failure is on the source side (because then there is no
s->error for migrate_fd_cleanup() to report).

In all, I’m asking:
(1) Is there a nice solution for me now to delay quitting qemu until the
failed migration has been fully resolved, including the clean-up?

(2) Isn’t it a problem if qemu crashes when you issue “quit” via QMP at
the wrong time?  Like, maybe lingering subprocesses when using “exec”?


Thanks,

Max


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

Re: Properly quitting qemu immediately after failing migration

[Vladimir Sementsov-Ogievskiy]

29.06.2020 16:48, Max Reitz wrote:
> Hi,
> 
> In an iotest, I’m trying to quit qemu immediately after a migration has
> failed.  Unfortunately, that doesn’t seem to be possible in a clean way:
> migrate_fd_cleanup() runs only at some point after the migration state
> is already “failed”, so if I just wait for that “failed” state and
> immediately quit, some cleanup functions may not have been run yet.
> 
> This is a problem with dirty bitmap migration at least, because it
> increases the refcount on all block devices that are to be migrated, so
> if we don’t call the cleanup function before quitting, the refcount will
> stay elevated and bdrv_close_all() will hit an assertion because those
> block devices are still around after blk_remove_all_bs() and
> blockdev_close_all_bdrv_states().
> 
> In practice this particular issue might not be that big of a problem,
> because it just means qemu aborts when the user intended to let it quit
> anyway.  But on one hand I could imagine that there are other clean-up
> paths that should definitely run before qemu quits (although I don’t
> know), and on the other, it’s a problem for my test.
> 
> I tried working around the problem for my test by waiting on “Unable to
> write” appearing on stderr, because that indicates that
> migrate_fd_cleanup()’s error_report_err() has been reached.  But on one
> hand, that isn’t really nice, and on the other, it doesn’t even work
> when the failure is on the source side (because then there is no
> s->error for migrate_fd_cleanup() to report).
> 
> In all, I’m asking:
> (1) Is there a nice solution for me now to delay quitting qemu until the
> failed migration has been fully resolved, including the clean-up?
> 
> (2) Isn’t it a problem if qemu crashes when you issue “quit” via QMP at
> the wrong time?  Like, maybe lingering subprocesses when using “exec”?
> 
> 

I'll look more closely tomorrow, but as a short answer: try my series
"[PATCH v2 00/22] Fix error handling during bitmap postcopy" it
handles different problems around migration failures & qemu shutdown,
probably it will help.


-- 
Best regards,
Vladimir

Re: Properly quitting qemu immediately after failing migration

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3218 bytes --]

On 29.06.20 16:18, Vladimir Sementsov-Ogievskiy wrote:
> 29.06.2020 16:48, Max Reitz wrote:
>> Hi,
>>
>> In an iotest, I’m trying to quit qemu immediately after a migration has
>> failed.  Unfortunately, that doesn’t seem to be possible in a clean way:
>> migrate_fd_cleanup() runs only at some point after the migration state
>> is already “failed”, so if I just wait for that “failed” state and
>> immediately quit, some cleanup functions may not have been run yet.
>>
>> This is a problem with dirty bitmap migration at least, because it
>> increases the refcount on all block devices that are to be migrated, so
>> if we don’t call the cleanup function before quitting, the refcount will
>> stay elevated and bdrv_close_all() will hit an assertion because those
>> block devices are still around after blk_remove_all_bs() and
>> blockdev_close_all_bdrv_states().
>>
>> In practice this particular issue might not be that big of a problem,
>> because it just means qemu aborts when the user intended to let it quit
>> anyway.  But on one hand I could imagine that there are other clean-up
>> paths that should definitely run before qemu quits (although I don’t
>> know), and on the other, it’s a problem for my test.
>>
>> I tried working around the problem for my test by waiting on “Unable to
>> write” appearing on stderr, because that indicates that
>> migrate_fd_cleanup()’s error_report_err() has been reached.  But on one
>> hand, that isn’t really nice, and on the other, it doesn’t even work
>> when the failure is on the source side (because then there is no
>> s->error for migrate_fd_cleanup() to report).

(I’ve now managed to work around it by invoking blockdev-del on a node
affected by bitmap migration until it succeeds, because blockdev-del can
only succeed once the bitmap migration code has dropped its reference to
it.)

>> In all, I’m asking:
>> (1) Is there a nice solution for me now to delay quitting qemu until the
>> failed migration has been fully resolved, including the clean-up?
>>
>> (2) Isn’t it a problem if qemu crashes when you issue “quit” via QMP at
>> the wrong time?  Like, maybe lingering subprocesses when using “exec”?
>>
>>
> 
> I'll look more closely tomorrow, but as a short answer: try my series
> "[PATCH v2 00/22] Fix error handling during bitmap postcopy" it
> handles different problems around migration failures & qemu shutdown,
> probably it will help.

Not, it doesn’t seem to.

I’m not sure what exactly that series addresses, but FWIW I’m hitting
the problem in non-postcopy migration.  What my simplest reproducer does is:

On the source VM:

blockdev-add node-name='foo' driver='null-co'
block-dirty-bitmap-add node='foo' name='bmap0'

(Launch destination VM with some -incoming, e.g.
-incoming 'exec: cat /tmp/mig_file')

Both on source and destination:

migrate-set-capabilities capabilities=[
    {capability='events', state=true},
    {capability='dirty-bitmaps', state=true}
]

On source:

migrate uri='exec: cat > /tmp/mig_file'

Then wait for a MIGRATION event with data/status == 'failed', and then
issue 'quit'.

Max


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

Re: Properly quitting qemu immediately after failing migration

[Vladimir Sementsov-Ogievskiy]

29.06.2020 18:00, Max Reitz wrote:
> On 29.06.20 16:18, Vladimir Sementsov-Ogievskiy wrote:
>> 29.06.2020 16:48, Max Reitz wrote:
>>> Hi,
>>>
>>> In an iotest, I’m trying to quit qemu immediately after a migration has
>>> failed.  Unfortunately, that doesn’t seem to be possible in a clean way:
>>> migrate_fd_cleanup() runs only at some point after the migration state
>>> is already “failed”, so if I just wait for that “failed” state and
>>> immediately quit, some cleanup functions may not have been run yet.
>>>
>>> This is a problem with dirty bitmap migration at least, because it
>>> increases the refcount on all block devices that are to be migrated, so
>>> if we don’t call the cleanup function before quitting, the refcount will
>>> stay elevated and bdrv_close_all() will hit an assertion because those
>>> block devices are still around after blk_remove_all_bs() and
>>> blockdev_close_all_bdrv_states().
>>>
>>> In practice this particular issue might not be that big of a problem,
>>> because it just means qemu aborts when the user intended to let it quit
>>> anyway.  But on one hand I could imagine that there are other clean-up
>>> paths that should definitely run before qemu quits (although I don’t
>>> know), and on the other, it’s a problem for my test.
>>>
>>> I tried working around the problem for my test by waiting on “Unable to
>>> write” appearing on stderr, because that indicates that
>>> migrate_fd_cleanup()’s error_report_err() has been reached.  But on one
>>> hand, that isn’t really nice, and on the other, it doesn’t even work
>>> when the failure is on the source side (because then there is no
>>> s->error for migrate_fd_cleanup() to report).
> 
> (I’ve now managed to work around it by invoking blockdev-del on a node
> affected by bitmap migration until it succeeds, because blockdev-del can
> only succeed once the bitmap migration code has dropped its reference to
> it.)
> 
>>> In all, I’m asking:
>>> (1) Is there a nice solution for me now to delay quitting qemu until the
>>> failed migration has been fully resolved, including the clean-up?
>>>
>>> (2) Isn’t it a problem if qemu crashes when you issue “quit” via QMP at
>>> the wrong time?  Like, maybe lingering subprocesses when using “exec”?
>>>
>>>
>>
>> I'll look more closely tomorrow, but as a short answer: try my series
>> "[PATCH v2 00/22] Fix error handling during bitmap postcopy" it
>> handles different problems around migration failures & qemu shutdown,
>> probably it will help.
> 
> Not, it doesn’t seem to.
> 
> I’m not sure what exactly that series addresses, but FWIW I’m hitting
> the problem in non-postcopy migration.  What my simplest reproducer does is:

Bitmaps migration is postcopy by nature (and it may not work without
migrate-start-postcopy, but work in most simple cases, as when we have
small bitmap data to migrate, it can migrate during migration downtime).
Most complicated part of the series were about postcopy. Still it fixes some other things.

It seems, that patch (see the second paragraph)
"[PATCH v2 10/22] migration/block-dirty-bitmap: cancel migration on shutdown"

>    If target is turned of prior to postcopy finished, target crashes
>    because busy bitmaps are found at shutdown.
>    Canceling incoming migration helps, as it removes all unfinished (and
>    therefore busy) bitmaps.

>    Similarly on source we crash in bdrv_close_all which asserts that all
>    bdrv states are removed, because bdrv states involved into dirty bitmap
>    migration are referenced by it. So, we need to cancel outgoing
>    migration as well.
     
should address exactly your issue.

> 
> On the source VM:
> 
> blockdev-add node-name='foo' driver='null-co'
> block-dirty-bitmap-add node='foo' name='bmap0'
> 
> (Launch destination VM with some -incoming, e.g.
> -incoming 'exec: cat /tmp/mig_file')
> 
> Both on source and destination:
> 
> migrate-set-capabilities capabilities=[
>      {capability='events', state=true},
>      {capability='dirty-bitmaps', state=true}
> ]
> 
> On source:
> 
> migrate uri='exec: cat > /tmp/mig_file'
> 
> Then wait for a MIGRATION event with data/status == 'failed', and then
> issue 'quit'.
> 
> Max
> 

Can you post exact reproducer iotest?

I've tried to reproduce by doing

diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 621a60e179..eeec47f97f 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -94,23 +94,6 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
  
          self.assertEqual(status == 'completed', migration_success)
          if status == 'failed':
-            # Wait until the migration has been cleaned up
-            # (Otherwise, bdrv_close_all() will abort because the
-            # dirty bitmap migration code still holds a reference to
-            # the BDS)
-            # (Unfortunately, there does not appear to be a nicer way
-            # of waiting until a failed migration has been cleaned up)
-            timeout_msg = 'Timeout waiting for migration to be cleaned up'
-            with iotests.Timeout(30, timeout_msg):
-                while os.path.exists(mig_sock):
-                    pass
-
-                # Dropping src_node_name will only work once the
-                # bitmap migration code has released it
-                while 'error' in self.vm_a.qmp('blockdev-del',
-                                               node_name=self.src_node_name):
-                    pass
-
              return
  
          self.vm_a.wait_for_runstate('postmigrate')


with your iotest, but it doesn't reproduce for me.



-- 
Best regards,
Vladimir

Re: Properly quitting qemu immediately after failing migration

[Max Reitz]

[-- Attachment #1.1.1: Type: text/plain, Size: 5244 bytes --]

On 01.07.20 18:16, Vladimir Sementsov-Ogievskiy wrote:
> 29.06.2020 18:00, Max Reitz wrote:
>> On 29.06.20 16:18, Vladimir Sementsov-Ogievskiy wrote:
>>> 29.06.2020 16:48, Max Reitz wrote:
>>>> Hi,
>>>>
>>>> In an iotest, I’m trying to quit qemu immediately after a migration has
>>>> failed.  Unfortunately, that doesn’t seem to be possible in a clean
>>>> way:
>>>> migrate_fd_cleanup() runs only at some point after the migration state
>>>> is already “failed”, so if I just wait for that “failed” state and
>>>> immediately quit, some cleanup functions may not have been run yet.
>>>>
>>>> This is a problem with dirty bitmap migration at least, because it
>>>> increases the refcount on all block devices that are to be migrated, so
>>>> if we don’t call the cleanup function before quitting, the refcount
>>>> will
>>>> stay elevated and bdrv_close_all() will hit an assertion because those
>>>> block devices are still around after blk_remove_all_bs() and
>>>> blockdev_close_all_bdrv_states().
>>>>
>>>> In practice this particular issue might not be that big of a problem,
>>>> because it just means qemu aborts when the user intended to let it quit
>>>> anyway.  But on one hand I could imagine that there are other clean-up
>>>> paths that should definitely run before qemu quits (although I don’t
>>>> know), and on the other, it’s a problem for my test.
>>>>
>>>> I tried working around the problem for my test by waiting on “Unable to
>>>> write” appearing on stderr, because that indicates that
>>>> migrate_fd_cleanup()’s error_report_err() has been reached.  But on one
>>>> hand, that isn’t really nice, and on the other, it doesn’t even work
>>>> when the failure is on the source side (because then there is no
>>>> s->error for migrate_fd_cleanup() to report).
>>
>> (I’ve now managed to work around it by invoking blockdev-del on a node
>> affected by bitmap migration until it succeeds, because blockdev-del can
>> only succeed once the bitmap migration code has dropped its reference to
>> it.)
>>
>>>> In all, I’m asking:
>>>> (1) Is there a nice solution for me now to delay quitting qemu until
>>>> the
>>>> failed migration has been fully resolved, including the clean-up?
>>>>
>>>> (2) Isn’t it a problem if qemu crashes when you issue “quit” via QMP at
>>>> the wrong time?  Like, maybe lingering subprocesses when using “exec”?
>>>>
>>>>
>>>
>>> I'll look more closely tomorrow, but as a short answer: try my series
>>> "[PATCH v2 00/22] Fix error handling during bitmap postcopy" it
>>> handles different problems around migration failures & qemu shutdown,
>>> probably it will help.
>>
>> Not, it doesn’t seem to.
>>
>> I’m not sure what exactly that series addresses, but FWIW I’m hitting
>> the problem in non-postcopy migration.  What my simplest reproducer
>> does is:
> 
> Bitmaps migration is postcopy by nature (and it may not work without
> migrate-start-postcopy, but work in most simple cases, as when we have
> small bitmap data to migrate, it can migrate during migration downtime).
> Most complicated part of the series were about postcopy. Still it fixes
> some other things.
> 
> It seems, that patch (see the second paragraph)
> "[PATCH v2 10/22] migration/block-dirty-bitmap: cancel migration on
> shutdown"
> 
>>    If target is turned of prior to postcopy finished, target crashes
>>    because busy bitmaps are found at shutdown.
>>    Canceling incoming migration helps, as it removes all unfinished (and
>>    therefore busy) bitmaps.
> 
>>    Similarly on source we crash in bdrv_close_all which asserts that all
>>    bdrv states are removed, because bdrv states involved into dirty
>> bitmap
>>    migration are referenced by it. So, we need to cancel outgoing
>>    migration as well.
>     should address exactly your issue.

Hm.  I’ve tested your series and still hit the issue.

I could imagine that my problem lies with a failed migration that is
automatically “cancelled” by nature, so the problem isn’t that it isn’t
cancelled, but that the clean-up runs after accepting further QMP
commands (like quit).

>>
>> On the source VM:
>>
>> blockdev-add node-name='foo' driver='null-co'
>> block-dirty-bitmap-add node='foo' name='bmap0'
>>
>> (Launch destination VM with some -incoming, e.g.
>> -incoming 'exec: cat /tmp/mig_file')
>>
>> Both on source and destination:
>>
>> migrate-set-capabilities capabilities=[
>>      {capability='events', state=true},
>>      {capability='dirty-bitmaps', state=true}
>> ]
>>
>> On source:
>>
>> migrate uri='exec: cat > /tmp/mig_file'
>>
>> Then wait for a MIGRATION event with data/status == 'failed', and then
>> issue 'quit'.
>>
>> Max
>>
> 
> Can you post exact reproducer iotest?

I didn’t have an iotest until now (because it was a simple test written
up in Ruby), but what I’ve attached should work.

Note that you need system load to trigger the problem (or the clean-up
code is scheduled too quickly), so I usually just run like three
instances concurrently.

(while TEST_DIR=/tmp/t$i ./check 400; do; done)

Max

[-- Attachment #1.1.2: 0001-Quit-crash-reproducer.patch --]
[-- Type: text/x-patch, Size: 2476 bytes --]

From ce0cacd21058f27fcb18aa632bfd5bc4fb3feadf Mon Sep 17 00:00:00 2001
From: Max Reitz <mreitz@redhat.com>
Date: Thu, 2 Jul 2020 09:21:14 +0200
Subject: [PATCH] Quit crash reproducer

---
 tests/qemu-iotests/400     | 42 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/400.out |  5 +++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 48 insertions(+)
 create mode 100755 tests/qemu-iotests/400
 create mode 100644 tests/qemu-iotests/400.out

diff --git a/tests/qemu-iotests/400 b/tests/qemu-iotests/400
new file mode 100755
index 0000000000..a32b2c3064
--- /dev/null
+++ b/tests/qemu-iotests/400
@@ -0,0 +1,42 @@
+#!/usr/bin/env python3
+
+import os
+import iotests
+
+mig_sock = os.path.join(iotests.sock_dir, 'mig.sock')
+
+class TestMigQuit(iotests.QMPTestCase):
+    def setUp(self):
+        self.vm_a = iotests.VM(path_suffix='a')
+        self.vm_a.launch()
+
+        self.vm_a.qmp('blockdev-add', node_name='foo', driver='null-co')
+        self.vm_a.qmp('block-dirty-bitmap-add', node='foo', name='bmap0')
+
+        self.vm_b = iotests.VM(path_suffix='b')
+        self.vm_b.add_incoming(f'unix:{mig_sock}')
+        self.vm_b.launch()
+
+        for vm in (self.vm_a, self.vm_b):
+            vm.qmp('migrate-set-capabilities',
+                    capabilities=[{'capability': 'events', 'state': True},
+                                  {'capability': 'dirty-bitmaps',
+                                   'state': True}])
+
+    def tearDown(self):
+        self.vm_a.shutdown()
+        self.vm_b.shutdown()
+
+        try:
+            os.remove(mig_sock)
+        except OSError:
+            pass
+
+    def test_mig_quit(self):
+        self.vm_a.qmp('migrate', uri=f'unix:{mig_sock}')
+
+        while self.vm_a.event_wait('MIGRATION')['data']['status'] != 'failed':
+            pass
+
+if __name__ == '__main__':
+    iotests.main()
diff --git a/tests/qemu-iotests/400.out b/tests/qemu-iotests/400.out
new file mode 100644
index 0000000000..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/400.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d886fa0cb3..cdb785b034 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -302,3 +302,4 @@
 291 rw quick
 292 rw auto quick
 297 meta
+400
-- 
2.26.2


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

Re: Properly quitting qemu immediately after failing migration

[Vladimir Sementsov-Ogievskiy]

02.07.2020 10:23, Max Reitz wrote:
> On 01.07.20 18:16, Vladimir Sementsov-Ogievskiy wrote:
>> 29.06.2020 18:00, Max Reitz wrote:
>>> On 29.06.20 16:18, Vladimir Sementsov-Ogievskiy wrote:
>>>> 29.06.2020 16:48, Max Reitz wrote:
>>>>> Hi,
>>>>>
>>>>> In an iotest, I’m trying to quit qemu immediately after a migration has
>>>>> failed.  Unfortunately, that doesn’t seem to be possible in a clean
>>>>> way:
>>>>> migrate_fd_cleanup() runs only at some point after the migration state
>>>>> is already “failed”, so if I just wait for that “failed” state and
>>>>> immediately quit, some cleanup functions may not have been run yet.
>>>>>
>>>>> This is a problem with dirty bitmap migration at least, because it
>>>>> increases the refcount on all block devices that are to be migrated, so
>>>>> if we don’t call the cleanup function before quitting, the refcount
>>>>> will
>>>>> stay elevated and bdrv_close_all() will hit an assertion because those
>>>>> block devices are still around after blk_remove_all_bs() and
>>>>> blockdev_close_all_bdrv_states().
>>>>>
>>>>> In practice this particular issue might not be that big of a problem,
>>>>> because it just means qemu aborts when the user intended to let it quit
>>>>> anyway.  But on one hand I could imagine that there are other clean-up
>>>>> paths that should definitely run before qemu quits (although I don’t
>>>>> know), and on the other, it’s a problem for my test.
>>>>>
>>>>> I tried working around the problem for my test by waiting on “Unable to
>>>>> write” appearing on stderr, because that indicates that
>>>>> migrate_fd_cleanup()’s error_report_err() has been reached.  But on one
>>>>> hand, that isn’t really nice, and on the other, it doesn’t even work
>>>>> when the failure is on the source side (because then there is no
>>>>> s->error for migrate_fd_cleanup() to report).
>>>
>>> (I’ve now managed to work around it by invoking blockdev-del on a node
>>> affected by bitmap migration until it succeeds, because blockdev-del can
>>> only succeed once the bitmap migration code has dropped its reference to
>>> it.)
>>>
>>>>> In all, I’m asking:
>>>>> (1) Is there a nice solution for me now to delay quitting qemu until
>>>>> the
>>>>> failed migration has been fully resolved, including the clean-up?
>>>>>
>>>>> (2) Isn’t it a problem if qemu crashes when you issue “quit” via QMP at
>>>>> the wrong time?  Like, maybe lingering subprocesses when using “exec”?
>>>>>
>>>>>
>>>>
>>>> I'll look more closely tomorrow, but as a short answer: try my series
>>>> "[PATCH v2 00/22] Fix error handling during bitmap postcopy" it
>>>> handles different problems around migration failures & qemu shutdown,
>>>> probably it will help.
>>>
>>> Not, it doesn’t seem to.
>>>
>>> I’m not sure what exactly that series addresses, but FWIW I’m hitting
>>> the problem in non-postcopy migration.  What my simplest reproducer
>>> does is:
>>
>> Bitmaps migration is postcopy by nature (and it may not work without
>> migrate-start-postcopy, but work in most simple cases, as when we have
>> small bitmap data to migrate, it can migrate during migration downtime).
>> Most complicated part of the series were about postcopy. Still it fixes
>> some other things.
>>
>> It seems, that patch (see the second paragraph)
>> "[PATCH v2 10/22] migration/block-dirty-bitmap: cancel migration on
>> shutdown"
>>
>>>     If target is turned of prior to postcopy finished, target crashes
>>>     because busy bitmaps are found at shutdown.
>>>     Canceling incoming migration helps, as it removes all unfinished (and
>>>     therefore busy) bitmaps.
>>
>>>     Similarly on source we crash in bdrv_close_all which asserts that all
>>>     bdrv states are removed, because bdrv states involved into dirty
>>> bitmap
>>>     migration are referenced by it. So, we need to cancel outgoing
>>>     migration as well.
>>      should address exactly your issue.
> 
> Hm.  I’ve tested your series and still hit the issue.
> 
> I could imagine that my problem lies with a failed migration that is
> automatically “cancelled” by nature, so the problem isn’t that it isn’t
> cancelled, but that the clean-up runs after accepting further QMP
> commands (like quit).

Looking at my patch I see

+void dirty_bitmap_mig_cancel_outgoing(void)
+{
+    dirty_bitmap_do_save_cleanup(&dbm_state.save);
+}
+

So, may be "cancel" is just a bad name. It should work, but it doesn't.

> 
>>>
>>> On the source VM:
>>>
>>> blockdev-add node-name='foo' driver='null-co'
>>> block-dirty-bitmap-add node='foo' name='bmap0'
>>>
>>> (Launch destination VM with some -incoming, e.g.
>>> -incoming 'exec: cat /tmp/mig_file')
>>>
>>> Both on source and destination:
>>>
>>> migrate-set-capabilities capabilities=[
>>>       {capability='events', state=true},
>>>       {capability='dirty-bitmaps', state=true}
>>> ]
>>>
>>> On source:
>>>
>>> migrate uri='exec: cat > /tmp/mig_file'
>>>
>>> Then wait for a MIGRATION event with data/status == 'failed', and then
>>> issue 'quit'.
>>>
>>> Max
>>>
>>
>> Can you post exact reproducer iotest?
> 
> I didn’t have an iotest until now (because it was a simple test written
> up in Ruby), but what I’ve attached should work.
> 
> Note that you need system load to trigger the problem (or the clean-up
> code is scheduled too quickly), so I usually just run like three
> instances concurrently.
> 
> (while TEST_DIR=/tmp/t$i ./check 400; do; done)
> 
> Max
> 

Thanks! Aha, reporoduced on your branch, more than 500 rolls, several (5-6) instances.

Interesting, if drop failure-waiting loop, it crashes without any race, just crashes.

Move to my branch.

With dropped fail-waiting loop, it crashes in about 17 tries, with several instances.

Ahahaha. and with fail-waiting loop as is, it crashes immediately, without a race.

So my patch make it work visa-versa. Magic.

For me this looks like my patch just don't do what it should. I'll work on this and
resend the series together with new test case. Or may be better to split the series,
to address different issues separately.

-- 
Best regards,
Vladimir

Re: Properly quitting qemu immediately after failing migration

[Vladimir Sementsov-Ogievskiy]

02.07.2020 14:44, Vladimir Sementsov-Ogievskiy wrote:
> 02.07.2020 10:23, Max Reitz wrote:
>> On 01.07.20 18:16, Vladimir Sementsov-Ogievskiy wrote:
>>> 29.06.2020 18:00, Max Reitz wrote:
>>>> On 29.06.20 16:18, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 29.06.2020 16:48, Max Reitz wrote:
>>>>>> Hi,
>>>>>>
>>>>>> In an iotest, I’m trying to quit qemu immediately after a migration has
>>>>>> failed.  Unfortunately, that doesn’t seem to be possible in a clean
>>>>>> way:
>>>>>> migrate_fd_cleanup() runs only at some point after the migration state
>>>>>> is already “failed”, so if I just wait for that “failed” state and
>>>>>> immediately quit, some cleanup functions may not have been run yet.
>>>>>>
>>>>>> This is a problem with dirty bitmap migration at least, because it
>>>>>> increases the refcount on all block devices that are to be migrated, so
>>>>>> if we don’t call the cleanup function before quitting, the refcount
>>>>>> will
>>>>>> stay elevated and bdrv_close_all() will hit an assertion because those
>>>>>> block devices are still around after blk_remove_all_bs() and
>>>>>> blockdev_close_all_bdrv_states().
>>>>>>
>>>>>> In practice this particular issue might not be that big of a problem,
>>>>>> because it just means qemu aborts when the user intended to let it quit
>>>>>> anyway.  But on one hand I could imagine that there are other clean-up
>>>>>> paths that should definitely run before qemu quits (although I don’t
>>>>>> know), and on the other, it’s a problem for my test.
>>>>>>
>>>>>> I tried working around the problem for my test by waiting on “Unable to
>>>>>> write” appearing on stderr, because that indicates that
>>>>>> migrate_fd_cleanup()’s error_report_err() has been reached.  But on one
>>>>>> hand, that isn’t really nice, and on the other, it doesn’t even work
>>>>>> when the failure is on the source side (because then there is no
>>>>>> s->error for migrate_fd_cleanup() to report).
>>>>
>>>> (I’ve now managed to work around it by invoking blockdev-del on a node
>>>> affected by bitmap migration until it succeeds, because blockdev-del can
>>>> only succeed once the bitmap migration code has dropped its reference to
>>>> it.)
>>>>
>>>>>> In all, I’m asking:
>>>>>> (1) Is there a nice solution for me now to delay quitting qemu until
>>>>>> the
>>>>>> failed migration has been fully resolved, including the clean-up?
>>>>>>
>>>>>> (2) Isn’t it a problem if qemu crashes when you issue “quit” via QMP at
>>>>>> the wrong time?  Like, maybe lingering subprocesses when using “exec”?
>>>>>>
>>>>>>
>>>>>
>>>>> I'll look more closely tomorrow, but as a short answer: try my series
>>>>> "[PATCH v2 00/22] Fix error handling during bitmap postcopy" it
>>>>> handles different problems around migration failures & qemu shutdown,
>>>>> probably it will help.
>>>>
>>>> Not, it doesn’t seem to.
>>>>
>>>> I’m not sure what exactly that series addresses, but FWIW I’m hitting
>>>> the problem in non-postcopy migration.  What my simplest reproducer
>>>> does is:
>>>
>>> Bitmaps migration is postcopy by nature (and it may not work without
>>> migrate-start-postcopy, but work in most simple cases, as when we have
>>> small bitmap data to migrate, it can migrate during migration downtime).
>>> Most complicated part of the series were about postcopy. Still it fixes
>>> some other things.
>>>
>>> It seems, that patch (see the second paragraph)
>>> "[PATCH v2 10/22] migration/block-dirty-bitmap: cancel migration on
>>> shutdown"
>>>
>>>>     If target is turned of prior to postcopy finished, target crashes
>>>>     because busy bitmaps are found at shutdown.
>>>>     Canceling incoming migration helps, as it removes all unfinished (and
>>>>     therefore busy) bitmaps.
>>>
>>>>     Similarly on source we crash in bdrv_close_all which asserts that all
>>>>     bdrv states are removed, because bdrv states involved into dirty
>>>> bitmap
>>>>     migration are referenced by it. So, we need to cancel outgoing
>>>>     migration as well.
>>>      should address exactly your issue.
>>
>> Hm.  I’ve tested your series and still hit the issue.
>>
>> I could imagine that my problem lies with a failed migration that is
>> automatically “cancelled” by nature, so the problem isn’t that it isn’t
>> cancelled, but that the clean-up runs after accepting further QMP
>> commands (like quit).
> 
> Looking at my patch I see
> 
> +void dirty_bitmap_mig_cancel_outgoing(void)
> +{
> +    dirty_bitmap_do_save_cleanup(&dbm_state.save);
> +}
> +
> 
> So, may be "cancel" is just a bad name. It should work, but it doesn't.
> 
>>
>>>>
>>>> On the source VM:
>>>>
>>>> blockdev-add node-name='foo' driver='null-co'
>>>> block-dirty-bitmap-add node='foo' name='bmap0'
>>>>
>>>> (Launch destination VM with some -incoming, e.g.
>>>> -incoming 'exec: cat /tmp/mig_file')
>>>>
>>>> Both on source and destination:
>>>>
>>>> migrate-set-capabilities capabilities=[
>>>>       {capability='events', state=true},
>>>>       {capability='dirty-bitmaps', state=true}
>>>> ]
>>>>
>>>> On source:
>>>>
>>>> migrate uri='exec: cat > /tmp/mig_file'
>>>>
>>>> Then wait for a MIGRATION event with data/status == 'failed', and then
>>>> issue 'quit'.
>>>>
>>>> Max
>>>>
>>>
>>> Can you post exact reproducer iotest?
>>
>> I didn’t have an iotest until now (because it was a simple test written
>> up in Ruby), but what I’ve attached should work.
>>
>> Note that you need system load to trigger the problem (or the clean-up
>> code is scheduled too quickly), so I usually just run like three
>> instances concurrently.
>>
>> (while TEST_DIR=/tmp/t$i ./check 400; do; done)
>>
>> Max
>>
> 
> Thanks! Aha, reporoduced on your branch, more than 500 rolls, several (5-6) instances.
> 
> Interesting, if drop failure-waiting loop, it crashes without any race, just crashes.
> 
> Move to my branch.
> 
> With dropped fail-waiting loop, it crashes in about 17 tries, with several instances.

very interesting: on crash-case dirty_bitmap_save_setup() is called after migration_shutdown().

So, migration_shutdown calls my dirty_bitmap_do_save_cleanup() which cleanups empty migration setup,
and then migration starts.

So, it's the race: If we do qmp migrate and then qmp quit, the actual sequence of operation may be changed. Not good.

I can hack it, just adding DBMSaveState.shutdown bool variable and set it to true in dirty_bitmap_do_save_cleanup
(probably, need distinguish shutdown from usual migration finish/cancel), and do nothing in all other bitmap
migration handlers, if shutdown is true.

But true way is ofcourse fixing generic migration code to not initiate a migration if it is already in shutdown..

Any ideas?

> 
> Ahahaha. and with fail-waiting loop as is, it crashes immediately, without a race.

Ok, this is another crash, simple to fix (and introduced by my series).

> 
> So my patch make it work visa-versa. Magic.
> 
> For me this looks like my patch just don't do what it should. I'll work on this and
> resend the series together with new test case. Or may be better to split the series,
> to address different issues separately.
> 


-- 
Best regards,
Vladimir

Re: Properly quitting qemu immediately after failing migration

[Dr. David Alan Gilbert]

* Max Reitz (mreitz@redhat.com) wrote:
> Hi,
> 
> In an iotest, I’m trying to quit qemu immediately after a migration has
> failed.  Unfortunately, that doesn’t seem to be possible in a clean way:
> migrate_fd_cleanup() runs only at some point after the migration state
> is already “failed”, so if I just wait for that “failed” state and
> immediately quit, some cleanup functions may not have been run yet.

Yeh this is hard; I always take the end of migrate_fd_cleanup to be the
real end.
It always happens on the main thread I think (it's done as a bh in some
cases).

> This is a problem with dirty bitmap migration at least, because it
> increases the refcount on all block devices that are to be migrated, so
> if we don’t call the cleanup function before quitting, the refcount will
> stay elevated and bdrv_close_all() will hit an assertion because those
> block devices are still around after blk_remove_all_bs() and
> blockdev_close_all_bdrv_states().
> 
> In practice this particular issue might not be that big of a problem,
> because it just means qemu aborts when the user intended to let it quit
> anyway.  But on one hand I could imagine that there are other clean-up
> paths that should definitely run before qemu quits (although I don’t
> know), and on the other, it’s a problem for my test.

'quit' varies - there are a lot of incoming failures that just assert;
very few of them cause a clean exit (I think there are more clean ones
after Peter's work on restartable postcopy a year or two ago).

I do see the end of migrate_fd_cleanup calls the notifier list; but it's
not clear to me that it's alwyas going to see the first transition to
'failed' at that point.

> I tried working around the problem for my test by waiting on “Unable to
> write” appearing on stderr, because that indicates that
> migrate_fd_cleanup()’s error_report_err() has been reached.  But on one
> hand, that isn’t really nice, and on the other, it doesn’t even work
> when the failure is on the source side (because then there is no
> s->error for migrate_fd_cleanup() to report).
> 
> In all, I’m asking:
> (1) Is there a nice solution for me now to delay quitting qemu until the
> failed migration has been fully resolved, including the clean-up?

In vl.c, I added a call to migration_shutdown in qemu_cleanup - although
that seems to be mostly about cleaning up the *outgoing* side; you could
add some incoming cleanup there.

> (2) Isn’t it a problem if qemu crashes when you issue “quit” via QMP at
> the wrong time?  Like, maybe lingering subprocesses when using “exec”?

Yeh that should be cleaner, but isn't.

Dave

> 
> Thanks,
> 
> Max
> 



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

Re: Properly quitting qemu immediately after failing migration

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3913 bytes --]

On 29.06.20 17:41, Dr. David Alan Gilbert wrote:
> * Max Reitz (mreitz@redhat.com) wrote:
>> Hi,
>>
>> In an iotest, I’m trying to quit qemu immediately after a migration has
>> failed.  Unfortunately, that doesn’t seem to be possible in a clean way:
>> migrate_fd_cleanup() runs only at some point after the migration state
>> is already “failed”, so if I just wait for that “failed” state and
>> immediately quit, some cleanup functions may not have been run yet.
> 
> Yeh this is hard; I always take the end of migrate_fd_cleanup to be the
> real end.

Yes, unfortunately I don’t seem to have a way to look for that end. :(

> It always happens on the main thread I think (it's done as a bh in some
> cases).
> 
>> This is a problem with dirty bitmap migration at least, because it
>> increases the refcount on all block devices that are to be migrated, so
>> if we don’t call the cleanup function before quitting, the refcount will
>> stay elevated and bdrv_close_all() will hit an assertion because those
>> block devices are still around after blk_remove_all_bs() and
>> blockdev_close_all_bdrv_states().
>>
>> In practice this particular issue might not be that big of a problem,
>> because it just means qemu aborts when the user intended to let it quit
>> anyway.  But on one hand I could imagine that there are other clean-up
>> paths that should definitely run before qemu quits (although I don’t
>> know), and on the other, it’s a problem for my test.
> 
> 'quit' varies - there are a lot of incoming failures that just assert;
> very few of them cause a clean exit (I think there are more clean ones
> after Peter's work on restartable postcopy a year or two ago).

Well, my problem is about the source side, where there is still a VM
running that I would expect to be in a sane state even after a failed
migration.

> I do see the end of migrate_fd_cleanup calls the notifier list; but it's
> not clear to me that it's alwyas going to see the first transition to
> 'failed' at that point.

What exactly do you mean?  It appears to me that both query-status and
the MIGRATION events signal the failed state before migrate_fd_cleanup()
is invoked.

If you mean I could add a notifier to that list to do something™, I’m
not sure what exactly it is I’d so.  My test can’t do it, because it’s
an iotest, and even if it could, I suppose I’d want to wait until even
after all notifiers have been invoked (which isn’t guaranteed if I’d add
a notifier myself).

>> I tried working around the problem for my test by waiting on “Unable to
>> write” appearing on stderr, because that indicates that
>> migrate_fd_cleanup()’s error_report_err() has been reached.  But on one
>> hand, that isn’t really nice, and on the other, it doesn’t even work
>> when the failure is on the source side (because then there is no
>> s->error for migrate_fd_cleanup() to report).
>>
>> In all, I’m asking:
>> (1) Is there a nice solution for me now to delay quitting qemu until the
>> failed migration has been fully resolved, including the clean-up?
> 
> In vl.c, I added a call to migration_shutdown in qemu_cleanup - although
> that seems to be mostly about cleaning up the *outgoing* side; you could
> add some incoming cleanup there.

So you mean waiting until migrate_fd_cleanup() has run?  Maybe I’ll try
that tomorrow, although I’d hoped I could get this done without having
to modify the code base...  (I.e., I’d hoped there would be some
QMP-queriable flag somewhere that could tell me whether the
migrate_fd_cleanup() has run)

>> (2) Isn’t it a problem if qemu crashes when you issue “quit” via QMP at
>> the wrong time?  Like, maybe lingering subprocesses when using “exec”?
> 
> Yeh that should be cleaner, but isn't.

:(

OK then.  Thanks for your insights!

Max


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

Re: Properly quitting qemu immediately after failing migration

[Dr. David Alan Gilbert]

* Max Reitz (mreitz@redhat.com) wrote:
> On 29.06.20 17:41, Dr. David Alan Gilbert wrote:
> > * Max Reitz (mreitz@redhat.com) wrote:
> >> Hi,
> >>
> >> In an iotest, I’m trying to quit qemu immediately after a migration has
> >> failed.  Unfortunately, that doesn’t seem to be possible in a clean way:
> >> migrate_fd_cleanup() runs only at some point after the migration state
> >> is already “failed”, so if I just wait for that “failed” state and
> >> immediately quit, some cleanup functions may not have been run yet.
> > 
> > Yeh this is hard; I always take the end of migrate_fd_cleanup to be the
> > real end.
> 
> Yes, unfortunately I don’t seem to have a way to look for that end. :(
> 
> > It always happens on the main thread I think (it's done as a bh in some
> > cases).
> > 
> >> This is a problem with dirty bitmap migration at least, because it
> >> increases the refcount on all block devices that are to be migrated, so
> >> if we don’t call the cleanup function before quitting, the refcount will
> >> stay elevated and bdrv_close_all() will hit an assertion because those
> >> block devices are still around after blk_remove_all_bs() and
> >> blockdev_close_all_bdrv_states().
> >>
> >> In practice this particular issue might not be that big of a problem,
> >> because it just means qemu aborts when the user intended to let it quit
> >> anyway.  But on one hand I could imagine that there are other clean-up
> >> paths that should definitely run before qemu quits (although I don’t
> >> know), and on the other, it’s a problem for my test.
> > 
> > 'quit' varies - there are a lot of incoming failures that just assert;
> > very few of them cause a clean exit (I think there are more clean ones
> > after Peter's work on restartable postcopy a year or two ago).
> 
> Well, my problem is about the source side, where there is still a VM
> running that I would expect to be in a sane state even after a failed
> migration.

Oh! Source side; the source side I worry much more about; if the
destination implodes after a failed migration I'm not too worried - but
the source side *shall not* fail.

> > I do see the end of migrate_fd_cleanup calls the notifier list; but it's
> > not clear to me that it's alwyas going to see the first transition to
> > 'failed' at that point.
> 
> What exactly do you mean?  It appears to me that both query-status and
> the MIGRATION events signal the failed state before migrate_fd_cleanup()
> is invoked.

OK, I was getting confused between event sending and the notifiers; the
event happens a bit before.  The state gets set to failed and then we
call fd_cleanup in most cases; the state is what query-status is keyed
off, and is also what causes the event to be sent.

> If you mean I could add a notifier to that list to do something™, I’m
> not sure what exactly it is I’d so.

Yeh, that's why the notifiers are there....
However, if you need to do some cleanup at the end of a migration, then
I think adding a call inside migrate_fd_cleanup is perfectly fine.

>  My test can’t do it, because it’s
> an iotest, and even if it could, I suppose I’d want to wait until even
> after all notifiers have been invoked (which isn’t guaranteed if I’d add
> a notifier myself).
> 
> >> I tried working around the problem for my test by waiting on “Unable to
> >> write” appearing on stderr, because that indicates that
> >> migrate_fd_cleanup()’s error_report_err() has been reached.  But on one
> >> hand, that isn’t really nice, and on the other, it doesn’t even work
> >> when the failure is on the source side (because then there is no
> >> s->error for migrate_fd_cleanup() to report).
> >>
> >> In all, I’m asking:
> >> (1) Is there a nice solution for me now to delay quitting qemu until the
> >> failed migration has been fully resolved, including the clean-up?
> > 
> > In vl.c, I added a call to migration_shutdown in qemu_cleanup - although
> > that seems to be mostly about cleaning up the *outgoing* side; you could
> > add some incoming cleanup there.
> 
> So you mean waiting until migrate_fd_cleanup() has run?  Maybe I’ll try
> that tomorrow, although I’d hoped I could get this done without having
> to modify the code base...  (I.e., I’d hoped there would be some
> QMP-queriable flag somewhere that could tell me whether the
> migrate_fd_cleanup() has run)

Please be a little careful around there; i.e. try as far as possible not
to hang on dead block devices.

> >> (2) Isn’t it a problem if qemu crashes when you issue “quit” via QMP at
> >> the wrong time?  Like, maybe lingering subprocesses when using “exec”?
> > 
> > Yeh that should be cleaner, but isn't.
> 
> :(

Randomly quitting is safer than it used to be (again that qemu_cleanup
code is what should make it so).

Dave

> OK then.  Thanks for your insights!
> 
> Max
> 



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

Re: Properly quitting qemu immediately after failing migration

[Daniel P. Berrangé]

On Mon, Jun 29, 2020 at 03:48:35PM +0200, Max Reitz wrote:
> In practice this particular issue might not be that big of a problem,
> because it just means qemu aborts when the user intended to let it quit
> anyway.  But on one hand I could imagine that there are other clean-up
> paths that should definitely run before qemu quits (although I don’t
> know), and on the other, it’s a problem for my test.

In general we can't assume any cleanup runs when incoming migration
fails, because when loading the migration stream, it often aborts with
asserts if the data doesn't match what's expected.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Properly quitting qemu immediately after failing migration

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 726 bytes --]

On 29.06.20 17:45, Daniel P. Berrangé wrote:
> On Mon, Jun 29, 2020 at 03:48:35PM +0200, Max Reitz wrote:
>> In practice this particular issue might not be that big of a problem,
>> because it just means qemu aborts when the user intended to let it quit
>> anyway.  But on one hand I could imagine that there are other clean-up
>> paths that should definitely run before qemu quits (although I don’t
>> know), and on the other, it’s a problem for my test.
> 
> In general we can't assume any cleanup runs when incoming migration
> fails, because when loading the migration stream, it often aborts with
> asserts if the data doesn't match what's expected.

My problem is about the source VM, though.

Max


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