[2/4] qmp-cont: invalidate on RUN_STATE_PRELAUNCH
diff mbox

Message ID 20170225193155.447462-3-vsementsov@virtuozzo.com
State New
Headers show

Commit Message

Vladimir Sementsov-Ogievskiy Feb. 25, 2017, 7:31 p.m. UTC
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(-)

Comments

Fam Zheng March 7, 2017, 9:19 a.m. UTC | #1
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>
Kevin Wolf March 7, 2017, 10:02 a.m. UTC | #2
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
>
Vladimir Sementsov-Ogievskiy March 7, 2017, 10:11 a.m. UTC | #3
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
>>
Kevin Wolf March 7, 2017, 10:22 a.m. UTC | #4
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
Kashyap Chamarthy April 26, 2017, 12:22 p.m. UTC | #5
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"
Vladimir Sementsov-Ogievskiy April 26, 2017, 1:43 p.m. UTC | #6
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.

Patch
diff mbox

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) {