| Message ID | 20170225193155.447462-3-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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 >
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 >>
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
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"
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.
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) {
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(-)