| Message ID | 20180208103132.28452-28-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,01/28] migration: better error handling with QEMUFile | expand |
* Peter Xu (peterx@redhat.com) wrote: > It pauses an ongoing migration. Currently it only supports postcopy. > Note that this command will work on either side of the migration. > Basically when we trigger this on one side, it'll interrupt the other > side as well since the other side will get notified on the disconnect > event. > > However, it's still possible that the other side is not notified, for > example, when the network is totally broken, or due to some firewall > configuration changes. In that case, we will also need to run the same > command on the other side so both sides will go into the paused state. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > migration/migration.c | 27 +++++++++++++++++++++++++++ > qapi/migration.json | 16 ++++++++++++++++ > 2 files changed, 43 insertions(+) > > diff --git a/migration/migration.c b/migration/migration.c > index bb57ed9ade..139abec0c3 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1448,6 +1448,33 @@ void qmp_migrate_recover(const char *uri, Error **errp) > qemu_start_incoming_migration(uri, errp); > } > > +void qmp_migrate_pause(Error **errp) > +{ > + MigrationState *ms = migrate_get_current(); > + MigrationIncomingState *mis = migration_incoming_get_current(); > + int ret; > + > + if (ms->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) { > + /* Source side, during postcopy */ > + ret = qemu_file_shutdown(ms->to_dst_file); This doesn't feel thread safe; although I'm not sure how to make it so. If the migration finishes just after we check the state but before the shutdown we end up using a bogus QEMUFile* Making all the places that close a QEMUFile* set hte pointer Null before they do the close doesn't help because you still race with that. (The race is small, but still) Dave > + if (ret) { > + error_setg(errp, "Failed to pause source migration"); > + } > + return; > + } > + > + if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) { > + ret = qemu_file_shutdown(mis->from_src_file); > + if (ret) { > + error_setg(errp, "Failed to pause destination migration"); > + } > + return; > + } > + > + error_setg(errp, "migrate-pause is currently only supported " > + "during postcopy-active state"); > +} > + > bool migration_is_blocked(Error **errp) > { > if (qemu_savevm_state_blocked(errp)) { > diff --git a/qapi/migration.json b/qapi/migration.json > index dfbcb02d4c..3d9cfeb8f1 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -1192,3 +1192,19 @@ > ## > { 'command': 'migrate-recover', 'data': { 'uri': 'str' }, > 'allow-oob': true } > + > +## > +# @migrate-pause: > +# > +# Pause a migration. Currently it only supports postcopy. > +# > +# Returns: nothing. > +# > +# Example: > +# > +# -> { "execute": "migrate-pause" } > +# <- { "return": {} } > +# > +# Since: 2.12 > +## > +{ 'command': 'migrate-pause', 'allow-oob': true } > -- > 2.14.3 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Tue, Feb 13, 2018 at 08:11:00PM +0000, Dr. David Alan Gilbert wrote: > * Peter Xu (peterx@redhat.com) wrote: > > It pauses an ongoing migration. Currently it only supports postcopy. > > Note that this command will work on either side of the migration. > > Basically when we trigger this on one side, it'll interrupt the other > > side as well since the other side will get notified on the disconnect > > event. > > > > However, it's still possible that the other side is not notified, for > > example, when the network is totally broken, or due to some firewall > > configuration changes. In that case, we will also need to run the same > > command on the other side so both sides will go into the paused state. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > migration/migration.c | 27 +++++++++++++++++++++++++++ > > qapi/migration.json | 16 ++++++++++++++++ > > 2 files changed, 43 insertions(+) > > > > diff --git a/migration/migration.c b/migration/migration.c > > index bb57ed9ade..139abec0c3 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -1448,6 +1448,33 @@ void qmp_migrate_recover(const char *uri, Error **errp) > > qemu_start_incoming_migration(uri, errp); > > } > > > > +void qmp_migrate_pause(Error **errp) > > +{ > > + MigrationState *ms = migrate_get_current(); > > + MigrationIncomingState *mis = migration_incoming_get_current(); > > + int ret; > > + > > + if (ms->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) { > > + /* Source side, during postcopy */ > > + ret = qemu_file_shutdown(ms->to_dst_file); > > This doesn't feel thread safe; although I'm not sure how to make it so. > If the migration finishes just after we check the state but before the > shutdown we end up using a bogus QEMUFile* > Making all the places that close a QEMUFile* set hte pointer Null before > they do the close doesn't help because you still race with that. > > (The race is small, but still) IMHO we can fix it by adding a migration lock for management code. If you see my previous migrate cleanup series, it's in my todo. ;) The basic idea is that we take the lock for critical paths (but not during most of the migration process). E.g., we may need the lock for: - very beginning of migration, during setup - reaching the end of migration - every single migration QMP command (since HMP calls them so HMP will also acquire the lock) - anywhere else I didn't mention that may necessary, e.g., when we change migrate state, meanwhile we do something else - basically that should be an "atomic operation", and we need the lock to make sure of that. For the recovery series, I would prefer that we ignore this issue for now - since this problem is there for quite a long time AFAICT in the whole migration code rather than this series only, and we need to solve it once and for all. Thanks,
* Peter Xu (peterx@redhat.com) wrote: > On Tue, Feb 13, 2018 at 08:11:00PM +0000, Dr. David Alan Gilbert wrote: > > * Peter Xu (peterx@redhat.com) wrote: > > > It pauses an ongoing migration. Currently it only supports postcopy. > > > Note that this command will work on either side of the migration. > > > Basically when we trigger this on one side, it'll interrupt the other > > > side as well since the other side will get notified on the disconnect > > > event. > > > > > > However, it's still possible that the other side is not notified, for > > > example, when the network is totally broken, or due to some firewall > > > configuration changes. In that case, we will also need to run the same > > > command on the other side so both sides will go into the paused state. > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > --- > > > migration/migration.c | 27 +++++++++++++++++++++++++++ > > > qapi/migration.json | 16 ++++++++++++++++ > > > 2 files changed, 43 insertions(+) > > > > > > diff --git a/migration/migration.c b/migration/migration.c > > > index bb57ed9ade..139abec0c3 100644 > > > --- a/migration/migration.c > > > +++ b/migration/migration.c > > > @@ -1448,6 +1448,33 @@ void qmp_migrate_recover(const char *uri, Error **errp) > > > qemu_start_incoming_migration(uri, errp); > > > } > > > > > > +void qmp_migrate_pause(Error **errp) > > > +{ > > > + MigrationState *ms = migrate_get_current(); > > > + MigrationIncomingState *mis = migration_incoming_get_current(); > > > + int ret; > > > + > > > + if (ms->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) { > > > + /* Source side, during postcopy */ > > > + ret = qemu_file_shutdown(ms->to_dst_file); > > > > This doesn't feel thread safe; although I'm not sure how to make it so. > > If the migration finishes just after we check the state but before the > > shutdown we end up using a bogus QEMUFile* > > Making all the places that close a QEMUFile* set hte pointer Null before > > they do the close doesn't help because you still race with that. > > > > (The race is small, but still) > > IMHO we can fix it by adding a migration lock for management code. If > you see my previous migrate cleanup series, it's in my todo. ;) > > The basic idea is that we take the lock for critical paths (but not > during most of the migration process). E.g., we may need the lock > for: > > - very beginning of migration, during setup > - reaching the end of migration > - every single migration QMP command (since HMP calls them so HMP will > also acquire the lock) > - anywhere else I didn't mention that may necessary, e.g., when we > change migrate state, meanwhile we do something else - basically > that should be an "atomic operation", and we need the lock to make > sure of that. But then we couldn't take that lock in an OOB command, you'd have to audit all of those places that took it to make sure it didn't do any of the things OOB commands aren't allowed to do. > For the recovery series, I would prefer that we ignore this issue for > now - since this problem is there for quite a long time AFAICT in the > whole migration code rather than this series only, and we need to > solve it once and for all. I don't think those problems happen (much) in the existing code, because everything is done in the main thread. That's one reason why the to_dst_file is closed in migrate_fd_cleanup which is normally closed in the back-half run on the main thread. One way would be to make the state go to POSTCOPY_PAUSED here; note that migrate_set_state already uses an atomic_cmpxchg to do the update. Dave > Thanks, > > -- > Peter Xu -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Wed, Feb 14, 2018 at 06:56:59PM +0000, Dr. David Alan Gilbert wrote: > * Peter Xu (peterx@redhat.com) wrote: > > On Tue, Feb 13, 2018 at 08:11:00PM +0000, Dr. David Alan Gilbert wrote: > > > * Peter Xu (peterx@redhat.com) wrote: > > > > It pauses an ongoing migration. Currently it only supports postcopy. > > > > Note that this command will work on either side of the migration. > > > > Basically when we trigger this on one side, it'll interrupt the other > > > > side as well since the other side will get notified on the disconnect > > > > event. > > > > > > > > However, it's still possible that the other side is not notified, for > > > > example, when the network is totally broken, or due to some firewall > > > > configuration changes. In that case, we will also need to run the same > > > > command on the other side so both sides will go into the paused state. > > > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > --- > > > > migration/migration.c | 27 +++++++++++++++++++++++++++ > > > > qapi/migration.json | 16 ++++++++++++++++ > > > > 2 files changed, 43 insertions(+) > > > > > > > > diff --git a/migration/migration.c b/migration/migration.c > > > > index bb57ed9ade..139abec0c3 100644 > > > > --- a/migration/migration.c > > > > +++ b/migration/migration.c > > > > @@ -1448,6 +1448,33 @@ void qmp_migrate_recover(const char *uri, Error **errp) > > > > qemu_start_incoming_migration(uri, errp); > > > > } > > > > > > > > +void qmp_migrate_pause(Error **errp) > > > > +{ > > > > + MigrationState *ms = migrate_get_current(); > > > > + MigrationIncomingState *mis = migration_incoming_get_current(); > > > > + int ret; > > > > + > > > > + if (ms->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) { > > > > + /* Source side, during postcopy */ > > > > + ret = qemu_file_shutdown(ms->to_dst_file); > > > > > > This doesn't feel thread safe; although I'm not sure how to make it so. > > > If the migration finishes just after we check the state but before the > > > shutdown we end up using a bogus QEMUFile* > > > Making all the places that close a QEMUFile* set hte pointer Null before > > > they do the close doesn't help because you still race with that. > > > > > > (The race is small, but still) > > > > IMHO we can fix it by adding a migration lock for management code. If > > you see my previous migrate cleanup series, it's in my todo. ;) > > > > The basic idea is that we take the lock for critical paths (but not > > during most of the migration process). E.g., we may need the lock > > for: > > > > - very beginning of migration, during setup > > - reaching the end of migration > > - every single migration QMP command (since HMP calls them so HMP will > > also acquire the lock) > > - anywhere else I didn't mention that may necessary, e.g., when we > > change migrate state, meanwhile we do something else - basically > > that should be an "atomic operation", and we need the lock to make > > sure of that. > > But then we couldn't take that lock in an OOB command, you'd have to > audit all of those places that took it to make sure it didn't do any of > the things OOB commands aren't allowed to do. Yeah OOB commands will be special - my plan is that they just never take the lock. E.g., they only touches FDs, and FDs are naturally thread safe (like this command). And some major migration commands (like "migrate" itself) should never be an OOB command. > > > For the recovery series, I would prefer that we ignore this issue for > > now - since this problem is there for quite a long time AFAICT in the > > whole migration code rather than this series only, and we need to > > solve it once and for all. > > I don't think those problems happen (much) in the existing code, because > everything is done in the main thread. But migration is running in its own thread (migration_thread)? For example: What if we send migration commands during the end of migration or a failing migration? Could there be risk in old code too since both main thread and migration thread may be manipulating MigrationState object? > That's one reason why the to_dst_file is closed in migrate_fd_cleanup > which is normally closed in the back-half run on the main thread. > > One way would be to make the state go to POSTCOPY_PAUSED here; > note that migrate_set_state already uses an atomic_cmpxchg to do the > update. Thanks,
* Peter Xu (peterx@redhat.com) wrote: > On Wed, Feb 14, 2018 at 06:56:59PM +0000, Dr. David Alan Gilbert wrote: > > * Peter Xu (peterx@redhat.com) wrote: > > > On Tue, Feb 13, 2018 at 08:11:00PM +0000, Dr. David Alan Gilbert wrote: > > > > * Peter Xu (peterx@redhat.com) wrote: > > > > > It pauses an ongoing migration. Currently it only supports postcopy. > > > > > Note that this command will work on either side of the migration. > > > > > Basically when we trigger this on one side, it'll interrupt the other > > > > > side as well since the other side will get notified on the disconnect > > > > > event. > > > > > > > > > > However, it's still possible that the other side is not notified, for > > > > > example, when the network is totally broken, or due to some firewall > > > > > configuration changes. In that case, we will also need to run the same > > > > > command on the other side so both sides will go into the paused state. > > > > > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > > --- > > > > > migration/migration.c | 27 +++++++++++++++++++++++++++ > > > > > qapi/migration.json | 16 ++++++++++++++++ > > > > > 2 files changed, 43 insertions(+) > > > > > > > > > > diff --git a/migration/migration.c b/migration/migration.c > > > > > index bb57ed9ade..139abec0c3 100644 > > > > > --- a/migration/migration.c > > > > > +++ b/migration/migration.c > > > > > @@ -1448,6 +1448,33 @@ void qmp_migrate_recover(const char *uri, Error **errp) > > > > > qemu_start_incoming_migration(uri, errp); > > > > > } > > > > > > > > > > +void qmp_migrate_pause(Error **errp) > > > > > +{ > > > > > + MigrationState *ms = migrate_get_current(); > > > > > + MigrationIncomingState *mis = migration_incoming_get_current(); > > > > > + int ret; > > > > > + > > > > > + if (ms->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) { > > > > > + /* Source side, during postcopy */ > > > > > + ret = qemu_file_shutdown(ms->to_dst_file); > > > > > > > > This doesn't feel thread safe; although I'm not sure how to make it so. > > > > If the migration finishes just after we check the state but before the > > > > shutdown we end up using a bogus QEMUFile* > > > > Making all the places that close a QEMUFile* set hte pointer Null before > > > > they do the close doesn't help because you still race with that. > > > > > > > > (The race is small, but still) > > > > > > IMHO we can fix it by adding a migration lock for management code. If > > > you see my previous migrate cleanup series, it's in my todo. ;) > > > > > > The basic idea is that we take the lock for critical paths (but not > > > during most of the migration process). E.g., we may need the lock > > > for: > > > > > > - very beginning of migration, during setup > > > - reaching the end of migration > > > - every single migration QMP command (since HMP calls them so HMP will > > > also acquire the lock) > > > - anywhere else I didn't mention that may necessary, e.g., when we > > > change migrate state, meanwhile we do something else - basically > > > that should be an "atomic operation", and we need the lock to make > > > sure of that. > > > > But then we couldn't take that lock in an OOB command, you'd have to > > audit all of those places that took it to make sure it didn't do any of > > the things OOB commands aren't allowed to do. > > Yeah OOB commands will be special - my plan is that they just never > take the lock. E.g., they only touches FDs, and FDs are naturally > thread safe (like this command). > > And some major migration commands (like "migrate" itself) should never > be an OOB command. OK; I'm not sure what makes FDs naturally thread safe though; but lets see the code you have in mind. > > > > > For the recovery series, I would prefer that we ignore this issue for > > > now - since this problem is there for quite a long time AFAICT in the > > > whole migration code rather than this series only, and we need to > > > solve it once and for all. > > > > I don't think those problems happen (much) in the existing code, because > > everything is done in the main thread. > > But migration is running in its own thread (migration_thread)? > > For example: What if we send migration commands during the end of > migration or a failing migration? Could there be risk in old code > too since both main thread and migration thread may be manipulating > MigrationState object? Maybe; although migrate_set_state uses atomic_cmpxchg to ensure that it's safe, and starting a migration can't happen unless there isn't a migration in progress - and that's run under lock. Dave > > That's one reason why the to_dst_file is closed in migrate_fd_cleanup > > which is normally closed in the back-half run on the main thread. > > > > One way would be to make the state go to POSTCOPY_PAUSED here; > > note that migrate_set_state already uses an atomic_cmpxchg to do the > > update. > > Thanks, > > -- > Peter Xu -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Wed, Feb 28, 2018 at 08:14:19PM +0000, Dr. David Alan Gilbert wrote: > * Peter Xu (peterx@redhat.com) wrote: > > On Wed, Feb 14, 2018 at 06:56:59PM +0000, Dr. David Alan Gilbert wrote: > > > * Peter Xu (peterx@redhat.com) wrote: > > > > On Tue, Feb 13, 2018 at 08:11:00PM +0000, Dr. David Alan Gilbert wrote: > > > > > * Peter Xu (peterx@redhat.com) wrote: > > > > > > It pauses an ongoing migration. Currently it only supports postcopy. > > > > > > Note that this command will work on either side of the migration. > > > > > > Basically when we trigger this on one side, it'll interrupt the other > > > > > > side as well since the other side will get notified on the disconnect > > > > > > event. > > > > > > > > > > > > However, it's still possible that the other side is not notified, for > > > > > > example, when the network is totally broken, or due to some firewall > > > > > > configuration changes. In that case, we will also need to run the same > > > > > > command on the other side so both sides will go into the paused state. > > > > > > > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > > > --- > > > > > > migration/migration.c | 27 +++++++++++++++++++++++++++ > > > > > > qapi/migration.json | 16 ++++++++++++++++ > > > > > > 2 files changed, 43 insertions(+) > > > > > > > > > > > > diff --git a/migration/migration.c b/migration/migration.c > > > > > > index bb57ed9ade..139abec0c3 100644 > > > > > > --- a/migration/migration.c > > > > > > +++ b/migration/migration.c > > > > > > @@ -1448,6 +1448,33 @@ void qmp_migrate_recover(const char *uri, Error **errp) > > > > > > qemu_start_incoming_migration(uri, errp); > > > > > > } > > > > > > > > > > > > +void qmp_migrate_pause(Error **errp) > > > > > > +{ > > > > > > + MigrationState *ms = migrate_get_current(); > > > > > > + MigrationIncomingState *mis = migration_incoming_get_current(); > > > > > > + int ret; > > > > > > + > > > > > > + if (ms->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) { > > > > > > + /* Source side, during postcopy */ > > > > > > + ret = qemu_file_shutdown(ms->to_dst_file); > > > > > > > > > > This doesn't feel thread safe; although I'm not sure how to make it so. > > > > > If the migration finishes just after we check the state but before the > > > > > shutdown we end up using a bogus QEMUFile* > > > > > Making all the places that close a QEMUFile* set hte pointer Null before > > > > > they do the close doesn't help because you still race with that. > > > > > > > > > > (The race is small, but still) > > > > > > > > IMHO we can fix it by adding a migration lock for management code. If > > > > you see my previous migrate cleanup series, it's in my todo. ;) > > > > > > > > The basic idea is that we take the lock for critical paths (but not > > > > during most of the migration process). E.g., we may need the lock > > > > for: > > > > > > > > - very beginning of migration, during setup > > > > - reaching the end of migration > > > > - every single migration QMP command (since HMP calls them so HMP will > > > > also acquire the lock) > > > > - anywhere else I didn't mention that may necessary, e.g., when we > > > > change migrate state, meanwhile we do something else - basically > > > > that should be an "atomic operation", and we need the lock to make > > > > sure of that. > > > > > > But then we couldn't take that lock in an OOB command, you'd have to > > > audit all of those places that took it to make sure it didn't do any of > > > the things OOB commands aren't allowed to do. > > > > Yeah OOB commands will be special - my plan is that they just never > > take the lock. E.g., they only touches FDs, and FDs are naturally > > thread safe (like this command). > > > > And some major migration commands (like "migrate" itself) should never > > be an OOB command. > > OK; I'm not sure what makes FDs naturally thread safe though; but > lets see the code you have in mind. I think I was wrong... it should need a lock. > > > > > > > > For the recovery series, I would prefer that we ignore this issue for > > > > now - since this problem is there for quite a long time AFAICT in the > > > > whole migration code rather than this series only, and we need to > > > > solve it once and for all. > > > > > > I don't think those problems happen (much) in the existing code, because > > > everything is done in the main thread. > > > > But migration is running in its own thread (migration_thread)? > > > > For example: What if we send migration commands during the end of > > migration or a failing migration? Could there be risk in old code > > too since both main thread and migration thread may be manipulating > > MigrationState object? > > Maybe; although migrate_set_state uses atomic_cmpxchg to ensure > that it's safe, and starting a migration can't happen unless there > isn't a migration in progress - and that's run under lock. Yes I think if without OOB we should be fine since even the cleanup is running with the BQL. Now I don't have good idea to solve this problem except introducing a lock. How about I add a patch to introduce the mgmt_lock, which currently only protect the QEMUFile? Like: ---------------------------------- diff --git a/migration/migration.c b/migration/migration.c index f31fcbb0d5..00c630326d 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1195,8 +1195,10 @@ static void migrate_fd_cleanup(void *opaque) if (multifd_save_cleanup(&local_err) != 0) { error_report_err(local_err); } + qemu_mutex_lock(&s->mgmt_lock); qemu_fclose(s->to_dst_file); s->to_dst_file = NULL; + qemu_mutex_unlock(&s->mgmt_lock); } assert((s->state != MIGRATION_STATUS_ACTIVE) && @@ -2493,8 +2495,10 @@ static MigThrError postcopy_pause(MigrationState *s) /* Current channel is possibly broken. Release it. */ assert(s->to_dst_file); + qemu_mutex_lock(&s->mgmt_lock); qemu_file_shutdown(s->to_dst_file); qemu_fclose(s->to_dst_file); s->to_dst_file = NULL; + qemu_mutex_unlock(&s->mgmt_lock); error_report("Detected IO failure for postcopy. " @@ -2970,6 +2974,7 @@ static void migration_instance_finalize(Object *obj) qemu_sem_destroy(&ms->postcopy_pause_sem); qemu_sem_destroy(&ms->postcopy_pause_rp_sem); qemu_sem_destroy(&ms->rp_state.rp_sem); + qemu_mutex_destroy(&ms->mgmt_lock); } static void migration_instance_init(Object *obj) @@ -3002,6 +3007,7 @@ static void migration_instance_init(Object *obj) qemu_sem_init(&ms->postcopy_pause_sem, 0); qemu_sem_init(&ms->postcopy_pause_rp_sem, 0); qemu_sem_init(&ms->rp_state.rp_sem, 0); + qemu_mutex_init(&ms->mgmt_lock); } /* diff --git a/migration/migration.h b/migration/migration.h index c549859cc3..7fcb841978 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -98,6 +98,11 @@ struct MigrationState QemuThread thread; QEMUBH *cleanup_bh; QEMUFile *to_dst_file; + /* + * Currently it only protects to_dst_file. We need to hold this + * lock when we want to modify in/out QEMUFiles. + */ + QemuMutex mgmt_lock; /* bytes already send at the beggining of current interation */ uint64_t iteration_initial_bytes; ---------------------------------- Then I take this lock in the OOB postcopy-pause handler. Since the lock holding scenarios are always extremely fast, I assume it's pretty safe. Would that be okay with you? Thanks,
* Peter Xu (peterx@redhat.com) wrote: > On Wed, Feb 28, 2018 at 08:14:19PM +0000, Dr. David Alan Gilbert wrote: > > * Peter Xu (peterx@redhat.com) wrote: > > > On Wed, Feb 14, 2018 at 06:56:59PM +0000, Dr. David Alan Gilbert wrote: > > > > * Peter Xu (peterx@redhat.com) wrote: > > > > > On Tue, Feb 13, 2018 at 08:11:00PM +0000, Dr. David Alan Gilbert wrote: > > > > > > * Peter Xu (peterx@redhat.com) wrote: > > > > > > > It pauses an ongoing migration. Currently it only supports postcopy. > > > > > > > Note that this command will work on either side of the migration. > > > > > > > Basically when we trigger this on one side, it'll interrupt the other > > > > > > > side as well since the other side will get notified on the disconnect > > > > > > > event. > > > > > > > > > > > > > > However, it's still possible that the other side is not notified, for > > > > > > > example, when the network is totally broken, or due to some firewall > > > > > > > configuration changes. In that case, we will also need to run the same > > > > > > > command on the other side so both sides will go into the paused state. > > > > > > > > > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > > > > --- > > > > > > > migration/migration.c | 27 +++++++++++++++++++++++++++ > > > > > > > qapi/migration.json | 16 ++++++++++++++++ > > > > > > > 2 files changed, 43 insertions(+) > > > > > > > > > > > > > > diff --git a/migration/migration.c b/migration/migration.c > > > > > > > index bb57ed9ade..139abec0c3 100644 > > > > > > > --- a/migration/migration.c > > > > > > > +++ b/migration/migration.c > > > > > > > @@ -1448,6 +1448,33 @@ void qmp_migrate_recover(const char *uri, Error **errp) > > > > > > > qemu_start_incoming_migration(uri, errp); > > > > > > > } > > > > > > > > > > > > > > +void qmp_migrate_pause(Error **errp) > > > > > > > +{ > > > > > > > + MigrationState *ms = migrate_get_current(); > > > > > > > + MigrationIncomingState *mis = migration_incoming_get_current(); > > > > > > > + int ret; > > > > > > > + > > > > > > > + if (ms->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) { > > > > > > > + /* Source side, during postcopy */ > > > > > > > + ret = qemu_file_shutdown(ms->to_dst_file); > > > > > > > > > > > > This doesn't feel thread safe; although I'm not sure how to make it so. > > > > > > If the migration finishes just after we check the state but before the > > > > > > shutdown we end up using a bogus QEMUFile* > > > > > > Making all the places that close a QEMUFile* set hte pointer Null before > > > > > > they do the close doesn't help because you still race with that. > > > > > > > > > > > > (The race is small, but still) > > > > > > > > > > IMHO we can fix it by adding a migration lock for management code. If > > > > > you see my previous migrate cleanup series, it's in my todo. ;) > > > > > > > > > > The basic idea is that we take the lock for critical paths (but not > > > > > during most of the migration process). E.g., we may need the lock > > > > > for: > > > > > > > > > > - very beginning of migration, during setup > > > > > - reaching the end of migration > > > > > - every single migration QMP command (since HMP calls them so HMP will > > > > > also acquire the lock) > > > > > - anywhere else I didn't mention that may necessary, e.g., when we > > > > > change migrate state, meanwhile we do something else - basically > > > > > that should be an "atomic operation", and we need the lock to make > > > > > sure of that. > > > > > > > > But then we couldn't take that lock in an OOB command, you'd have to > > > > audit all of those places that took it to make sure it didn't do any of > > > > the things OOB commands aren't allowed to do. > > > > > > Yeah OOB commands will be special - my plan is that they just never > > > take the lock. E.g., they only touches FDs, and FDs are naturally > > > thread safe (like this command). > > > > > > And some major migration commands (like "migrate" itself) should never > > > be an OOB command. > > > > OK; I'm not sure what makes FDs naturally thread safe though; but > > lets see the code you have in mind. > > I think I was wrong... it should need a lock. > > > > > > > > > > > > For the recovery series, I would prefer that we ignore this issue for > > > > > now - since this problem is there for quite a long time AFAICT in the > > > > > whole migration code rather than this series only, and we need to > > > > > solve it once and for all. > > > > > > > > I don't think those problems happen (much) in the existing code, because > > > > everything is done in the main thread. > > > > > > But migration is running in its own thread (migration_thread)? > > > > > > For example: What if we send migration commands during the end of > > > migration or a failing migration? Could there be risk in old code > > > too since both main thread and migration thread may be manipulating > > > MigrationState object? > > > > Maybe; although migrate_set_state uses atomic_cmpxchg to ensure > > that it's safe, and starting a migration can't happen unless there > > isn't a migration in progress - and that's run under lock. > > Yes I think if without OOB we should be fine since even the cleanup is > running with the BQL. > > Now I don't have good idea to solve this problem except introducing a > lock. How about I add a patch to introduce the mgmt_lock, which > currently only protect the QEMUFile? Like: > > ---------------------------------- > diff --git a/migration/migration.c b/migration/migration.c > index f31fcbb0d5..00c630326d 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1195,8 +1195,10 @@ static void migrate_fd_cleanup(void *opaque) > if (multifd_save_cleanup(&local_err) != 0) { > error_report_err(local_err); > } > + qemu_mutex_lock(&s->mgmt_lock); > qemu_fclose(s->to_dst_file); > s->to_dst_file = NULL; > + qemu_mutex_unlock(&s->mgmt_lock); > } > > assert((s->state != MIGRATION_STATUS_ACTIVE) && > @@ -2493,8 +2495,10 @@ static MigThrError postcopy_pause(MigrationState *s) > > /* Current channel is possibly broken. Release it. */ > assert(s->to_dst_file); > + qemu_mutex_lock(&s->mgmt_lock); > qemu_file_shutdown(s->to_dst_file); > qemu_fclose(s->to_dst_file); > s->to_dst_file = NULL; > + qemu_mutex_unlock(&s->mgmt_lock); That's only safe if we know qemu_fclose() can never block; otherwise we're not allowed to take the same lock in the OOB command. I think perhaps it's safer to always do something like: tmp = atomic_xchg(s->to_dst_file, NULL); qemu_file_shutdown(tmp); qemu_fclose(tmp); then the OOB code can do the same? Would that work - avoiding the lock? Dave > error_report("Detected IO failure for postcopy. " > @@ -2970,6 +2974,7 @@ static void migration_instance_finalize(Object *obj) > qemu_sem_destroy(&ms->postcopy_pause_sem); > qemu_sem_destroy(&ms->postcopy_pause_rp_sem); > qemu_sem_destroy(&ms->rp_state.rp_sem); > + qemu_mutex_destroy(&ms->mgmt_lock); > } > > static void migration_instance_init(Object *obj) > @@ -3002,6 +3007,7 @@ static void migration_instance_init(Object *obj) > qemu_sem_init(&ms->postcopy_pause_sem, 0); > qemu_sem_init(&ms->postcopy_pause_rp_sem, 0); > qemu_sem_init(&ms->rp_state.rp_sem, 0); > + qemu_mutex_init(&ms->mgmt_lock); > } > > /* > diff --git a/migration/migration.h b/migration/migration.h > index c549859cc3..7fcb841978 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -98,6 +98,11 @@ struct MigrationState > QemuThread thread; > QEMUBH *cleanup_bh; > QEMUFile *to_dst_file; > + /* > + * Currently it only protects to_dst_file. We need to hold this > + * lock when we want to modify in/out QEMUFiles. > + */ > + QemuMutex mgmt_lock; > > /* bytes already send at the beggining of current interation */ > uint64_t iteration_initial_bytes; > ---------------------------------- > > Then I take this lock in the OOB postcopy-pause handler. Since the > lock holding scenarios are always extremely fast, I assume it's pretty > safe. Would that be okay with you? > > Thanks, > > -- > Peter Xu -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Tue, Mar 13, 2018 at 12:35:56PM +0000, Dr. David Alan Gilbert wrote: [...] > > Yes I think if without OOB we should be fine since even the cleanup is > > running with the BQL. > > > > Now I don't have good idea to solve this problem except introducing a > > lock. How about I add a patch to introduce the mgmt_lock, which > > currently only protect the QEMUFile? Like: > > > > ---------------------------------- > > diff --git a/migration/migration.c b/migration/migration.c > > index f31fcbb0d5..00c630326d 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -1195,8 +1195,10 @@ static void migrate_fd_cleanup(void *opaque) > > if (multifd_save_cleanup(&local_err) != 0) { > > error_report_err(local_err); > > } > > + qemu_mutex_lock(&s->mgmt_lock); > > qemu_fclose(s->to_dst_file); > > s->to_dst_file = NULL; > > + qemu_mutex_unlock(&s->mgmt_lock); > > } > > > > assert((s->state != MIGRATION_STATUS_ACTIVE) && > > @@ -2493,8 +2495,10 @@ static MigThrError postcopy_pause(MigrationState *s) > > > > /* Current channel is possibly broken. Release it. */ > > assert(s->to_dst_file); > > + qemu_mutex_lock(&s->mgmt_lock); > > qemu_file_shutdown(s->to_dst_file); > > qemu_fclose(s->to_dst_file); > > s->to_dst_file = NULL; > > + qemu_mutex_unlock(&s->mgmt_lock); > > That's only safe if we know qemu_fclose() can never block; otherwise > we're not allowed to take the same lock in the OOB command. > > I think perhaps it's safer to always do something like: > tmp = atomic_xchg(s->to_dst_file, NULL); > qemu_file_shutdown(tmp); > qemu_fclose(tmp); > > then the OOB code can do the same? > Would that work - avoiding the lock? According to what we discussed offlist: I'll still introduce that lock, but instead I'll move the close() out of the lock section since that can block. I'll see whether I need a repost tomorrow, or after 2.12 release if the series will never have a chance for 2.12. Thanks,
* Peter Xu (peterx@redhat.com) wrote: > On Tue, Mar 13, 2018 at 12:35:56PM +0000, Dr. David Alan Gilbert wrote: > > [...] > > > > Yes I think if without OOB we should be fine since even the cleanup is > > > running with the BQL. > > > > > > Now I don't have good idea to solve this problem except introducing a > > > lock. How about I add a patch to introduce the mgmt_lock, which > > > currently only protect the QEMUFile? Like: > > > > > > ---------------------------------- > > > diff --git a/migration/migration.c b/migration/migration.c > > > index f31fcbb0d5..00c630326d 100644 > > > --- a/migration/migration.c > > > +++ b/migration/migration.c > > > @@ -1195,8 +1195,10 @@ static void migrate_fd_cleanup(void *opaque) > > > if (multifd_save_cleanup(&local_err) != 0) { > > > error_report_err(local_err); > > > } > > > + qemu_mutex_lock(&s->mgmt_lock); > > > qemu_fclose(s->to_dst_file); > > > s->to_dst_file = NULL; > > > + qemu_mutex_unlock(&s->mgmt_lock); > > > } > > > > > > assert((s->state != MIGRATION_STATUS_ACTIVE) && > > > @@ -2493,8 +2495,10 @@ static MigThrError postcopy_pause(MigrationState *s) > > > > > > /* Current channel is possibly broken. Release it. */ > > > assert(s->to_dst_file); > > > + qemu_mutex_lock(&s->mgmt_lock); > > > qemu_file_shutdown(s->to_dst_file); > > > qemu_fclose(s->to_dst_file); > > > s->to_dst_file = NULL; > > > + qemu_mutex_unlock(&s->mgmt_lock); > > > > That's only safe if we know qemu_fclose() can never block; otherwise > > we're not allowed to take the same lock in the OOB command. > > > > I think perhaps it's safer to always do something like: > > tmp = atomic_xchg(s->to_dst_file, NULL); > > qemu_file_shutdown(tmp); > > qemu_fclose(tmp); > > > > then the OOB code can do the same? > > Would that work - avoiding the lock? > > According to what we discussed offlist: I'll still introduce that > lock, but instead I'll move the close() out of the lock section since > that can block. Yep, it feels like that should work. > I'll see whether I need a repost tomorrow, or after 2.12 release if > the series will never have a chance for 2.12. Yes, I think the boat has probably sailed on 2.12; still with the OOB stuff in, things are much closer. Dave > Thanks, > > -- > Peter Xu -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/migration/migration.c b/migration/migration.c index bb57ed9ade..139abec0c3 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1448,6 +1448,33 @@ void qmp_migrate_recover(const char *uri, Error **errp) qemu_start_incoming_migration(uri, errp); } +void qmp_migrate_pause(Error **errp) +{ + MigrationState *ms = migrate_get_current(); + MigrationIncomingState *mis = migration_incoming_get_current(); + int ret; + + if (ms->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) { + /* Source side, during postcopy */ + ret = qemu_file_shutdown(ms->to_dst_file); + if (ret) { + error_setg(errp, "Failed to pause source migration"); + } + return; + } + + if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) { + ret = qemu_file_shutdown(mis->from_src_file); + if (ret) { + error_setg(errp, "Failed to pause destination migration"); + } + return; + } + + error_setg(errp, "migrate-pause is currently only supported " + "during postcopy-active state"); +} + bool migration_is_blocked(Error **errp) { if (qemu_savevm_state_blocked(errp)) { diff --git a/qapi/migration.json b/qapi/migration.json index dfbcb02d4c..3d9cfeb8f1 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1192,3 +1192,19 @@ ## { 'command': 'migrate-recover', 'data': { 'uri': 'str' }, 'allow-oob': true } + +## +# @migrate-pause: +# +# Pause a migration. Currently it only supports postcopy. +# +# Returns: nothing. +# +# Example: +# +# -> { "execute": "migrate-pause" } +# <- { "return": {} } +# +# Since: 2.12 +## +{ 'command': 'migrate-pause', 'allow-oob': true }
It pauses an ongoing migration. Currently it only supports postcopy. Note that this command will work on either side of the migration. Basically when we trigger this on one side, it'll interrupt the other side as well since the other side will get notified on the disconnect event. However, it's still possible that the other side is not notified, for example, when the network is totally broken, or due to some firewall configuration changes. In that case, we will also need to run the same command on the other side so both sides will go into the paused state. Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.c | 27 +++++++++++++++++++++++++++ qapi/migration.json | 16 ++++++++++++++++ 2 files changed, 43 insertions(+)