| Message ID | 20230428194928.1426370-10-vsementsov@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | COLO: improve build options | expand |
On Fri, Apr 28, 2023 at 10:49:27PM +0300, Vladimir Sementsov-Ogievskiy wrote: > COLO is not listed as running state in migrate_is_running(), so, it's > theoretically possible to disable colo capability in COLO state and the > unexpected error in migration_iteration_finish() is reachable. > > Let's disallow that in qmp_migrate_set_capabilities. Than the error > becomes absolutely unreachable: we can get into COLO state only with > enabled capability and can't disable it while we are in COLO state. So > substitute the error by simple assertion. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Peter Xu <peterx@redhat.com>
> -----Original Message----- > From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > Sent: Saturday, April 29, 2023 3:49 AM > To: qemu-devel@nongnu.org > Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen > <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Peter Xu > <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com> > Subject: [PATCH v4 09/10] migration: disallow change capabilities in COLO > state > > COLO is not listed as running state in migrate_is_running(), so, it's > theoretically possible to disable colo capability in COLO state and the > unexpected error in migration_iteration_finish() is reachable. > > Let's disallow that in qmp_migrate_set_capabilities. Than the error becomes > absolutely unreachable: we can get into COLO state only with enabled > capability and can't disable it while we are in COLO state. So substitute the > error by simple assertion. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > --- > migration/migration.c | 5 +---- > migration/options.c | 2 +- > 2 files changed, 2 insertions(+), 5 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c index > 0d912ee0d7..8c5bbf3e94 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -2751,10 +2751,7 @@ static void > migration_iteration_finish(MigrationState *s) > runstate_set(RUN_STATE_POSTMIGRATE); > break; > case MIGRATION_STATUS_COLO: > - if (!migrate_colo()) { > - error_report("%s: critical error: calling COLO code without " > - "COLO enabled", __func__); > - } > + assert(migrate_colo()); > migrate_start_colo_process(s); > s->vm_was_running = true; > /* Fallthrough */ > diff --git a/migration/options.c b/migration/options.c index > 865a0214d8..f461d02eeb 100644 > --- a/migration/options.c > +++ b/migration/options.c > @@ -598,7 +598,7 @@ void > qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params, > MigrationCapabilityStatusList *cap; > bool new_caps[MIGRATION_CAPABILITY__MAX]; > > - if (migration_is_running(s->state)) { > + if (migration_is_running(s->state) || migration_in_colo_state()) { Make the "MIGRATION_STATUS_COLO" into the " migration_is_running()" is a better way? Like the "migration_is_setup_ot_active()". Thanks Chen > error_setg(errp, QERR_MIGRATION_ACTIVE); > return; > } > -- > 2.34.1
On 04.05.23 11:09, Zhang, Chen wrote: > > >> -----Original Message----- >> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> Sent: Saturday, April 29, 2023 3:49 AM >> To: qemu-devel@nongnu.org >> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen >> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Peter Xu >> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com> >> Subject: [PATCH v4 09/10] migration: disallow change capabilities in COLO >> state >> >> COLO is not listed as running state in migrate_is_running(), so, it's >> theoretically possible to disable colo capability in COLO state and the >> unexpected error in migration_iteration_finish() is reachable. >> >> Let's disallow that in qmp_migrate_set_capabilities. Than the error becomes >> absolutely unreachable: we can get into COLO state only with enabled >> capability and can't disable it while we are in COLO state. So substitute the >> error by simple assertion. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> --- >> migration/migration.c | 5 +---- >> migration/options.c | 2 +- >> 2 files changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c index >> 0d912ee0d7..8c5bbf3e94 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -2751,10 +2751,7 @@ static void >> migration_iteration_finish(MigrationState *s) >> runstate_set(RUN_STATE_POSTMIGRATE); >> break; >> case MIGRATION_STATUS_COLO: >> - if (!migrate_colo()) { >> - error_report("%s: critical error: calling COLO code without " >> - "COLO enabled", __func__); >> - } >> + assert(migrate_colo()); >> migrate_start_colo_process(s); >> s->vm_was_running = true; >> /* Fallthrough */ >> diff --git a/migration/options.c b/migration/options.c index >> 865a0214d8..f461d02eeb 100644 >> --- a/migration/options.c >> +++ b/migration/options.c >> @@ -598,7 +598,7 @@ void >> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params, >> MigrationCapabilityStatusList *cap; >> bool new_caps[MIGRATION_CAPABILITY__MAX]; >> >> - if (migration_is_running(s->state)) { >> + if (migration_is_running(s->state) || migration_in_colo_state()) { > > Make the "MIGRATION_STATUS_COLO" into the " migration_is_running()" is a better way? I wasn't sure that that's correct.. migration_is_running() is used in several places, to do so, I'd have to analyze them all. > Like the "migration_is_setup_ot_active()". > > Thanks > Chen > >> error_setg(errp, QERR_MIGRATION_ACTIVE); >> return; >> } >> -- >> 2.34.1 >
> -----Original Message----- > From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > Sent: Thursday, May 4, 2023 4:23 PM > To: Zhang, Chen <chen.zhang@intel.com>; qemu-devel@nongnu.org > Cc: lukasstraub2@web.de; quintela@redhat.com; Peter Xu > <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com> > Subject: Re: [PATCH v4 09/10] migration: disallow change capabilities in COLO > state > > On 04.05.23 11:09, Zhang, Chen wrote: > > > > > >> -----Original Message----- > >> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > >> Sent: Saturday, April 29, 2023 3:49 AM > >> To: qemu-devel@nongnu.org > >> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen > >> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Peter Xu > >> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com> > >> Subject: [PATCH v4 09/10] migration: disallow change capabilities in > >> COLO state > >> > >> COLO is not listed as running state in migrate_is_running(), so, it's > >> theoretically possible to disable colo capability in COLO state and > >> the unexpected error in migration_iteration_finish() is reachable. > >> > >> Let's disallow that in qmp_migrate_set_capabilities. Than the error > >> becomes absolutely unreachable: we can get into COLO state only with > >> enabled capability and can't disable it while we are in COLO state. > >> So substitute the error by simple assertion. > >> > >> Signed-off-by: Vladimir Sementsov-Ogievskiy > >> <vsementsov@yandex-team.ru> > >> --- > >> migration/migration.c | 5 +---- > >> migration/options.c | 2 +- > >> 2 files changed, 2 insertions(+), 5 deletions(-) > >> > >> diff --git a/migration/migration.c b/migration/migration.c index > >> 0d912ee0d7..8c5bbf3e94 100644 > >> --- a/migration/migration.c > >> +++ b/migration/migration.c > >> @@ -2751,10 +2751,7 @@ static void > >> migration_iteration_finish(MigrationState *s) > >> runstate_set(RUN_STATE_POSTMIGRATE); > >> break; > >> case MIGRATION_STATUS_COLO: > >> - if (!migrate_colo()) { > >> - error_report("%s: critical error: calling COLO code without " > >> - "COLO enabled", __func__); > >> - } > >> + assert(migrate_colo()); > >> migrate_start_colo_process(s); > >> s->vm_was_running = true; > >> /* Fallthrough */ > >> diff --git a/migration/options.c b/migration/options.c index > >> 865a0214d8..f461d02eeb 100644 > >> --- a/migration/options.c > >> +++ b/migration/options.c > >> @@ -598,7 +598,7 @@ void > >> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params, > >> MigrationCapabilityStatusList *cap; > >> bool new_caps[MIGRATION_CAPABILITY__MAX]; > >> > >> - if (migration_is_running(s->state)) { > >> + if (migration_is_running(s->state) || migration_in_colo_state()) > >> + { > > > > Make the "MIGRATION_STATUS_COLO" into the " migration_is_running()" > is a better way? > > I wasn't sure that that's correct.. migration_is_running() is used in several > places, to do so, I'd have to analyze them all. Actually, when running in the "MIGRATION_STATUS_COLO" means QEMU can not do a normal migration at the same time. Juan have any comments here? Thanks Chen > > > Like the "migration_is_setup_ot_active()". > > > > Thanks > > Chen > > > >> error_setg(errp, QERR_MIGRATION_ACTIVE); > >> return; > >> } > >> -- > >> 2.34.1 > > > > -- > Best regards, > Vladimir
"Zhang, Chen" <chen.zhang@intel.com> wrote: >> -----Original Message----- >> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> Sent: Thursday, May 4, 2023 4:23 PM >> To: Zhang, Chen <chen.zhang@intel.com>; qemu-devel@nongnu.org >> Cc: lukasstraub2@web.de; quintela@redhat.com; Peter Xu >> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com> >> Subject: Re: [PATCH v4 09/10] migration: disallow change capabilities in COLO >> state >> >> On 04.05.23 11:09, Zhang, Chen wrote: >> > >> > >> >> -----Original Message----- >> >> From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> >> Sent: Saturday, April 29, 2023 3:49 AM >> >> To: qemu-devel@nongnu.org >> >> Cc: lukasstraub2@web.de; quintela@redhat.com; Zhang, Chen >> >> <chen.zhang@intel.com>; vsementsov@yandex-team.ru; Peter Xu >> >> <peterx@redhat.com>; Leonardo Bras <leobras@redhat.com> >> >> Subject: [PATCH v4 09/10] migration: disallow change capabilities in >> >> COLO state >> >> >> >> COLO is not listed as running state in migrate_is_running(), so, it's >> >> theoretically possible to disable colo capability in COLO state and >> >> the unexpected error in migration_iteration_finish() is reachable. >> >> >> >> Let's disallow that in qmp_migrate_set_capabilities. Than the error >> >> becomes absolutely unreachable: we can get into COLO state only with >> >> enabled capability and can't disable it while we are in COLO state. >> >> So substitute the error by simple assertion. >> >> >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> >> <vsementsov@yandex-team.ru> >> >> --- >> >> migration/migration.c | 5 +---- >> >> migration/options.c | 2 +- >> >> 2 files changed, 2 insertions(+), 5 deletions(-) >> >> >> >> diff --git a/migration/migration.c b/migration/migration.c index >> >> 0d912ee0d7..8c5bbf3e94 100644 >> >> --- a/migration/migration.c >> >> +++ b/migration/migration.c >> >> @@ -2751,10 +2751,7 @@ static void >> >> migration_iteration_finish(MigrationState *s) >> >> runstate_set(RUN_STATE_POSTMIGRATE); >> >> break; >> >> case MIGRATION_STATUS_COLO: >> >> - if (!migrate_colo()) { >> >> - error_report("%s: critical error: calling COLO code without " >> >> - "COLO enabled", __func__); >> >> - } >> >> + assert(migrate_colo()); >> >> migrate_start_colo_process(s); >> >> s->vm_was_running = true; >> >> /* Fallthrough */ >> >> diff --git a/migration/options.c b/migration/options.c index >> >> 865a0214d8..f461d02eeb 100644 >> >> --- a/migration/options.c >> >> +++ b/migration/options.c >> >> @@ -598,7 +598,7 @@ void >> >> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params, >> >> MigrationCapabilityStatusList *cap; >> >> bool new_caps[MIGRATION_CAPABILITY__MAX]; >> >> >> >> - if (migration_is_running(s->state)) { >> >> + if (migration_is_running(s->state) || migration_in_colo_state()) >> >> + { >> > >> > Make the "MIGRATION_STATUS_COLO" into the " migration_is_running()" >> is a better way? >> >> I wasn't sure that that's correct.. migration_is_running() is used in several >> places, to do so, I'd have to analyze them all. > > Actually, when running in the "MIGRATION_STATUS_COLO" means QEMU can not > do a normal migration at the same time. > Juan have any comments here? My understanding of colo is pretty limited, but my mind explodes just trying to think how many things we would have to check/do to do a migration while in colo state. So I guess you are right that we can't be in both at the same time. Later, Juan.
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: > COLO is not listed as running state in migrate_is_running(), so, it's > theoretically possible to disable colo capability in COLO state and the > unexpected error in migration_iteration_finish() is reachable. > > Let's disallow that in qmp_migrate_set_capabilities. Than the error > becomes absolutely unreachable: we can get into COLO state only with > enabled capability and can't disable it while we are in COLO state. So > substitute the error by simple assertion. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Juan Quintela <quintela@redhat.com>
diff --git a/migration/migration.c b/migration/migration.c index 0d912ee0d7..8c5bbf3e94 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2751,10 +2751,7 @@ static void migration_iteration_finish(MigrationState *s) runstate_set(RUN_STATE_POSTMIGRATE); break; case MIGRATION_STATUS_COLO: - if (!migrate_colo()) { - error_report("%s: critical error: calling COLO code without " - "COLO enabled", __func__); - } + assert(migrate_colo()); migrate_start_colo_process(s); s->vm_was_running = true; /* Fallthrough */ diff --git a/migration/options.c b/migration/options.c index 865a0214d8..f461d02eeb 100644 --- a/migration/options.c +++ b/migration/options.c @@ -598,7 +598,7 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params, MigrationCapabilityStatusList *cap; bool new_caps[MIGRATION_CAPABILITY__MAX]; - if (migration_is_running(s->state)) { + if (migration_is_running(s->state) || migration_in_colo_state()) { error_setg(errp, QERR_MIGRATION_ACTIVE); return; }
COLO is not listed as running state in migrate_is_running(), so, it's theoretically possible to disable colo capability in COLO state and the unexpected error in migration_iteration_finish() is reachable. Let's disallow that in qmp_migrate_set_capabilities. Than the error becomes absolutely unreachable: we can get into COLO state only with enabled capability and can't disable it while we are in COLO state. So substitute the error by simple assertion. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- migration/migration.c | 5 +---- migration/options.c | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-)