[Qemu-devel] [PATCH 0/2] fix qmp-cont vs migration-finish race-crash

[Qemu-devel] [PATCH 0/2] fix qmp-cont vs migration-finish race-crash

[Vladimir Sementsov-Ogievskiy]

Hi.

It's a simple fix for problems reported in "Aborts in iotest 169"
by Max:
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05907.html

In thread Kevin described that a problem itself is bigger and needs
more effort:
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06136.html

So, we may continue discussion in "Aborts in iotest 169", and in
parallel apply these patches at least as a temporary fix.

The problem of this fix is that we finally have a bit weird interface:

User gets event MIGRATION_COMPLETED, and after it he can get error
message "Migration is not finalized yet". 

But it is better than crash, anyway.

Vladimir Sementsov-Ogievskiy (2):
  qmp: forbid qmp_cont in RUN_STATE_FINISH_MIGRATE
  iotest: fix 169: do not run qmp_cont in RUN_STATE_FINISH_MIGRATE

 qmp.c                  | 3 +++
 tests/qemu-iotests/169 | 7 ++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

-- 
2.18.0

[Qemu-devel] [PATCH 1/2] qmp: forbid qmp_cont in RUN_STATE_FINISH_MIGRATE

[Vladimir Sementsov-Ogievskiy]

qmp_cont in RUN_STATE_FINISH_MIGRATE may lead to moving vm to
RUN_STATE_RUNNING, before actual migration finish. So, when migration
thread will try to go to RUN_STATE_POSTMIGRATE, assuming transition
RUN_STATE_FINISH_MIGRATE->RUN_STATE_POSTMIGRATE, it will crash, as
current state is RUN_STATE_RUNNING, and transition
RUN_STATE_RUNNING->RUN_STATE_POSTMIGRATE is forbidden.

Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qmp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qmp.c b/qmp.c
index 4c819dd8cf..c2ecf1d804 100644
--- a/qmp.c
+++ b/qmp.c
@@ -156,6 +156,9 @@ void qmp_cont(Error **errp)
         return;
     } else if (runstate_check(RUN_STATE_SUSPENDED)) {
         return;
+    } else if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
+        error_setg(errp, "Migration is not finalized yet");
+        return;
     }
 
     for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
-- 
2.18.0

Re: [Qemu-devel] [PATCH 1/2] qmp: forbid qmp_cont in RUN_STATE_FINISH_MIGRATE

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> qmp_cont in RUN_STATE_FINISH_MIGRATE may lead to moving vm to
> RUN_STATE_RUNNING, before actual migration finish. So, when migration
> thread will try to go to RUN_STATE_POSTMIGRATE, assuming transition
> RUN_STATE_FINISH_MIGRATE->RUN_STATE_POSTMIGRATE, it will crash, as
> current state is RUN_STATE_RUNNING, and transition
> RUN_STATE_RUNNING->RUN_STATE_POSTMIGRATE is forbidden.
> 
> Reported-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qmp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/qmp.c b/qmp.c
> index 4c819dd8cf..c2ecf1d804 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -156,6 +156,9 @@ void qmp_cont(Error **errp)
>          return;
>      } else if (runstate_check(RUN_STATE_SUSPENDED)) {
>          return;
> +    } else if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
> +        error_setg(errp, "Migration is not finalized yet");
> +        return;
>      }

Yeh, a bit of a hack, but I think that's as best as we can do for now;
we can't ban all cont's during migration.

Note, it's still racy since you could call this partway through the
completion before it's set to finish.


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


>      for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> -- 
> 2.18.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 2/2] iotest: fix 169: do not run qmp_cont in RUN_STATE_FINISH_MIGRATE

[Vladimir Sementsov-Ogievskiy]

qmp_cont fails if vm in RUN_STATE_FINISH_MIGRATE, so let's wait for
final RUN_STATE_POSTMIGRATE. Also, while being here, check qmp_cont
result.

Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/169 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
index 527aebd0cb..7e06cc1145 100755
--- a/tests/qemu-iotests/169
+++ b/tests/qemu-iotests/169
@@ -102,12 +102,17 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
             event = self.vm_a.event_wait('MIGRATION')
             if event['data']['status'] == 'completed':
                 break
+        while True:
+            result = self.vm_a.qmp('query-status')
+            if (result['return']['status'] == 'postmigrate'):
+                break
 
         # test that bitmap is still here
         removed = (not migrate_bitmaps) and persistent
         self.check_bitmap(self.vm_a, False if removed else sha256)
 
-        self.vm_a.qmp('cont')
+        result = self.vm_a.qmp('cont')
+        self.assert_qmp(result, 'return', {})
 
         # test that bitmap is still here after invalidation
         self.check_bitmap(self.vm_a, sha256)
-- 
2.18.0

Re: [Qemu-devel] [PATCH 2/2] iotest: fix 169: do not run qmp_cont in RUN_STATE_FINISH_MIGRATE

[Vladimir Sementsov-Ogievskiy]

24.01.2019 15:25, Vladimir Sementsov-Ogievskiy wrote:
> qmp_cont fails if vm in RUN_STATE_FINISH_MIGRATE, so let's wait for
> final RUN_STATE_POSTMIGRATE. Also, while being here, check qmp_cont
> result.
> 
> Reported-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/169 | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
> index 527aebd0cb..7e06cc1145 100755
> --- a/tests/qemu-iotests/169
> +++ b/tests/qemu-iotests/169
> @@ -102,12 +102,17 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>               event = self.vm_a.event_wait('MIGRATION')
>               if event['data']['status'] == 'completed':
>                   break
> +        while True:
> +            result = self.vm_a.qmp('query-status')
> +            if (result['return']['status'] == 'postmigrate'):
> +                break

possibly, not bad to add

+              time.sleep(0.1)

or 0.5, or something. (Note, that time module is already imported to the test)

>   
>           # test that bitmap is still here
>           removed = (not migrate_bitmaps) and persistent
>           self.check_bitmap(self.vm_a, False if removed else sha256)
>   
> -        self.vm_a.qmp('cont')
> +        result = self.vm_a.qmp('cont')
> +        self.assert_qmp(result, 'return', {})
>   
>           # test that bitmap is still here after invalidation
>           self.check_bitmap(self.vm_a, sha256)
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/2] iotest: fix 169: do not run qmp_cont in RUN_STATE_FINISH_MIGRATE

[Max Reitz]

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

On 24.01.19 13:25, Vladimir Sementsov-Ogievskiy wrote:
> qmp_cont fails if vm in RUN_STATE_FINISH_MIGRATE, so let's wait for
> final RUN_STATE_POSTMIGRATE. Also, while being here, check qmp_cont
> result.
> 
> Reported-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/169 | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Tested-by: Max Reitz <mreitz@redhat.com>

Thanks!


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

Re: [Qemu-devel] [PATCH 0/2] fix qmp-cont vs migration-finish race-crash

[Thomas Huth]

On 24/01/2019 13.25, Vladimir Sementsov-Ogievskiy wrote:
> Hi.
> 
> It's a simple fix for problems reported in "Aborts in iotest 169"
> by Max:
> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05907.html
> 
> In thread Kevin described that a problem itself is bigger and needs
> more effort:
> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06136.html
> 
> So, we may continue discussion in "Aborts in iotest 169", and in
> parallel apply these patches at least as a temporary fix.
> 
> The problem of this fix is that we finally have a bit weird interface:
> 
> User gets event MIGRATION_COMPLETED, and after it he can get error
> message "Migration is not finalized yet". 
> 
> But it is better than crash, anyway.
> 
> Vladimir Sementsov-Ogievskiy (2):
>   qmp: forbid qmp_cont in RUN_STATE_FINISH_MIGRATE
>   iotest: fix 169: do not run qmp_cont in RUN_STATE_FINISH_MIGRATE

What happened to these two patches? As far as I can see, they've never
got applied? Has another fix for 169 included instead?

 Thomas

Re: [Qemu-devel] [PATCH 0/2] fix qmp-cont vs migration-finish race-crash

[Dr. David Alan Gilbert]

* Thomas Huth (thuth@redhat.com) wrote:
> On 24/01/2019 13.25, Vladimir Sementsov-Ogievskiy wrote:
> > Hi.
> > 
> > It's a simple fix for problems reported in "Aborts in iotest 169"
> > by Max:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05907.html
> > 
> > In thread Kevin described that a problem itself is bigger and needs
> > more effort:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06136.html
> > 
> > So, we may continue discussion in "Aborts in iotest 169", and in
> > parallel apply these patches at least as a temporary fix.
> > 
> > The problem of this fix is that we finally have a bit weird interface:
> > 
> > User gets event MIGRATION_COMPLETED, and after it he can get error
> > message "Migration is not finalized yet". 
> > 
> > But it is better than crash, anyway.
> > 
> > Vladimir Sementsov-Ogievskiy (2):
> >   qmp: forbid qmp_cont in RUN_STATE_FINISH_MIGRATE
> >   iotest: fix 169: do not run qmp_cont in RUN_STATE_FINISH_MIGRATE
> 
> What happened to these two patches? As far as I can see, they've never
> got applied? Has another fix for 169 included instead?

Hmm not sure; they've slipped between the maintainers if not.
1/2 is a qmp.c so while it fixes a migration corner case it's probably
armbru that should take it.

Dave

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

Re: [Qemu-devel] [PATCH 0/2] fix qmp-cont vs migration-finish race-crash

[Kevin Wolf]

Am 24.01.2019 um 13:25 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi.
> 
> It's a simple fix for problems reported in "Aborts in iotest 169"
> by Max:
> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05907.html
> 
> In thread Kevin described that a problem itself is bigger and needs
> more effort:
> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06136.html
> 
> So, we may continue discussion in "Aborts in iotest 169", and in
> parallel apply these patches at least as a temporary fix.
> 
> The problem of this fix is that we finally have a bit weird interface:
> 
> User gets event MIGRATION_COMPLETED, and after it he can get error
> message "Migration is not finalized yet". 
> 
> But it is better than crash, anyway.

Thanks, applied to the block branch.

Kevin