From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56216) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDArP-000231-S8 for qemu-devel@nongnu.org; Wed, 15 Jun 2016 09:28:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bDArJ-0004UO-NO for qemu-devel@nongnu.org; Wed, 15 Jun 2016 09:27:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40454) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDArJ-0004UD-Ez for qemu-devel@nongnu.org; Wed, 15 Jun 2016 09:27:53 -0400 Date: Wed, 15 Jun 2016 14:27:49 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20160615132748.GG2272@work-vm> References: <1465409615-16365-1-git-send-email-haris.phnx@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1465409615-16365-1-git-send-email-haris.phnx@gmail.com> Subject: Re: [Qemu-devel] [Qemu-devel [RFC] [WIP] v1] Adding feature to reconnect with -r option to migrate command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Md Haris Iqbal Cc: qemu-devel@nongnu.org * Md Haris Iqbal (haris.phnx@gmail.com) wrote: > --- > hmp-commands.hx | 10 +++++--- > hmp.c | 4 ++- > include/migration/migration.h | 1 + > migration/migration.c | 60 +++++++++++++++++++++++++++++++++---------- > qapi-schema.json | 2 +- > qmp-commands.hx | 3 ++- > 6 files changed, 60 insertions(+), 20 deletions(-) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 4f4f60a..2e7aed3 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -893,10 +893,11 @@ ETEXI > > { > .name = "migrate", > - .args_type = "detach:-d,blk:-b,inc:-i,uri:s", > - .params = "[-d] [-b] [-i] uri", > + .args_type = "detach:-d,recover:-r,blk:-b,inc:-i,uri:s", > + .params = "[-d] [-r] [-b] [-i] uri", > .help = "migrate to URI (using -d to not wait for completion)" > - "\n\t\t\t -b for migration without shared storage with" > + "\n\t\t\t -r to recover from a broken migration\n\t\t\t" > + " -b for migration without shared storage with" Is that a tab/spce indent problem? > " full copy of disk\n\t\t\t -i for migration without " > "shared storage with incremental copy of disk " > "(base image shared between src and destination)", > @@ -905,9 +906,10 @@ ETEXI > > > STEXI > -@item migrate [-d] [-b] [-i] @var{uri} > +@item migrate [-d] [-r] [-b] [-i] @var{uri} > @findex migrate > Migrate to @var{uri} (using -d to not wait for completion). > + -r to recover from a broken migration Is that a tab/spce indent problem? > -b for migration with full copy of disk > -i for migration with incremental copy of disk (base image is shared) > ETEXI > diff --git a/hmp.c b/hmp.c > index d510236..ec8fab4 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1544,12 +1544,14 @@ static void hmp_migrate_status_cb(void *opaque) > void hmp_migrate(Monitor *mon, const QDict *qdict) > { > bool detach = qdict_get_try_bool(qdict, "detach", false); > + bool recover = qdict_get_try_bool(qdict, "recover", false); > bool blk = qdict_get_try_bool(qdict, "blk", false); > bool inc = qdict_get_try_bool(qdict, "inc", false); > const char *uri = qdict_get_str(qdict, "uri"); > Error *err = NULL; > > - qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err); > + qmp_migrate(uri, !!recover, recover, !!blk, blk, !!inc, inc, false, false, > + &err); > if (err) { > error_report_err(err); > return; > diff --git a/include/migration/migration.h b/include/migration/migration.h > index ac2c12c..4a3201b 100644 > --- a/include/migration/migration.h > +++ b/include/migration/migration.h > @@ -139,6 +139,7 @@ struct MigrationState > > int state; > MigrationParams params; > + bool in_recovery; > > /* State related to return path */ > struct { > diff --git a/migration/migration.c b/migration/migration.c > index 991313a..a77f62e 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -933,6 +933,7 @@ MigrationState *migrate_init(const MigrationParams *params) > s->xfer_limit = 0; > s->cleanup_bh = 0; > s->to_dst_file = NULL; > + s->in_recovery = false; > s->state = MIGRATION_STATUS_NONE; > s->params = *params; > s->rp_state.from_dst_file = NULL; > @@ -992,13 +993,14 @@ void qmp_migrate_incoming(const char *uri, Error **errp) > once = false; > } > > -void qmp_migrate(const char *uri, bool has_blk, bool blk, > - bool has_inc, bool inc, bool has_detach, bool detach, > +void qmp_migrate(const char *uri, bool in_recover, bool recover, bool has_blk, > + bool blk, bool has_inc, bool inc, bool has_detach, bool detach, > Error **errp) > { > Error *local_err = NULL; > MigrationState *s = migrate_get_current(); > MigrationParams params; > + bool recovery = in_recover && recover; > const char *p; You have an odd mix of things called recover and recovery. > > params.blk = has_blk && blk; > @@ -1023,7 +1025,31 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > return; > } > > - s = migrate_init(¶ms); > + if (recovery ^ atomic_mb_read(&s->in_recovery)) { > + if (recovery) { > + /* No VM is waiting for recovery and > + * recovery option was set > + */ > + > + error_setg(errp, QERR_UNDEFINED_ERROR); > + return; > + } else { > + /* A VM is waiting for recovery and > + * no recovery option is set > + */ > + > + error_setg(errp, QERR_UNDEFINED_ERROR); > + return; Use error messages that explain the problem; e.g. error_setg(errp, "Can't start a new migration while a migration is in recovery"); But I wonder; do you actually need an s->in_recovery? Can't you just use the current migration state - it's postcopy_recovery then is that the same as your in_recovery flag? Do we change state once we start the recovery, at the moment I think you're going: postcopy_active -> postmigrate_recovery but should we go to : postcopy_active -> postmigrate_recovery -> postmigrate_recovery_active ? > + } > + } else { > + if (!recovery) { > + /* No VM is waiting for recovery and > + * no recovery option is set > + */ > + fprintf(stderr, "hello1\n"); > + s = migrate_init(¶ms); > + } > + } > > if (strstart(uri, "tcp:", &p)) { > tcp_start_outgoing_migration(s, p, &local_err); > @@ -1767,16 +1793,6 @@ static void *migration_thread(void *opaque) > > void migrate_fd_connect(MigrationState *s) > { > - /* This is a best 1st approximation. ns to ms */ > - s->expected_downtime = max_downtime/1000000; > - s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s); > - > - qemu_file_set_rate_limit(s->to_dst_file, > - s->bandwidth_limit / XFER_LIMIT_RATIO); > - > - /* Notify before starting migration thread */ > - notifier_list_notify(&migration_state_notifiers, s); > - > /* > * Open the return path; currently for postcopy but other things might > * also want it. > @@ -1791,6 +1807,24 @@ void migrate_fd_connect(MigrationState *s) > } > } > > + if (atomic_mb_read(&s->in_recovery)) { > + atomic_mb_set(&s->in_recovery, 0); > + > + fprintf(stderr, "recovered\n"); > + return; > + } Mark the places where you're going to add stuff with /* TODO: explanation */ > + > + /* This is a best 1st approximation. ns to ms */ > + s->expected_downtime = max_downtime/1000000; > + s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s); > + > + qemu_file_set_rate_limit(s->to_dst_file, > + s->bandwidth_limit / XFER_LIMIT_RATIO); > + > + /* Notify before starting migration thread */ > + notifier_list_notify(&migration_state_notifiers, s); > + > + > migrate_compress_threads_create(); > qemu_thread_create(&s->thread, "migration", migration_thread, s, > QEMU_THREAD_JOINABLE); > diff --git a/qapi-schema.json b/qapi-schema.json > index 54634c4..1b7b1e1 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2070,7 +2070,7 @@ > # Since: 0.14.0 > ## > { 'command': 'migrate', > - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } } > + 'data': {'uri': 'str', '*recover': 'bool', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } } > > ## > # @migrate-incoming > diff --git a/qmp-commands.hx b/qmp-commands.hx > index de896a5..c8a1a93 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -612,7 +612,7 @@ EQMP > > { > .name = "migrate", > - .args_type = "detach:-d,blk:-b,inc:-i,uri:s", > + .args_type = "detach:-d,recover:-r,blk:-b,inc:-i,uri:s", > .mhandler.cmd_new = qmp_marshal_migrate, > }, > > @@ -625,6 +625,7 @@ Migrate to URI. > Arguments: > > - "blk": block migration, full disk copy (json-bool, optional) > +- "recover": recover migration (json-bool, optional) > - "inc": incremental disk copy (json-bool, optional) > - "uri": Destination URI (json-string) > > -- > 2.7.4 > Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK