From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
To: Jens Freimann <jfreimann@redhat.com>
Cc: pkrempa@redhat.com, berrange@redhat.com, ehabkost@redhat.com,
mst@redhat.com, aadam@redhat.com, jasowang@redhat.com,
qemu-devel@nongnu.org, armbru@redhat.com,
alex.williamson@redhat.com, laine@redhat.com, ailan@redhat.com,
parav@mellanox.com
Subject: Re: [PATCH v6 08/11] migration: add new migration state wait-unplug
Date: Tue, 29 Oct 2019 02:56:50 +0000 [thread overview]
Message-ID: <20191029025650.GE2508@work-vm> (raw)
In-Reply-To: <20191025121930.6855-9-jfreimann@redhat.com>
* Jens Freimann (jfreimann@redhat.com) wrote:
> This patch adds a new migration state called wait-unplug. It is entered
> after the SETUP state if failover devices are present. It will transition
> into ACTIVE once all devices were succesfully unplugged from the guest.
>
> So if a guest doesn't respond or takes long to honor the unplug request
> the user will see the migration state 'wait-unplug'.
>
> In the migration thread we query failover devices if they're are still
> pending the guest unplug. When all are unplugged the migration
> continues. If one device won't unplug migration will stay in wait_unplug
> state.
>
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> Acked-by: Cornelia Huck <cohuck@redhat.com>
I think this is OK, so
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
but see question below
> ---
> include/migration/vmstate.h | 2 ++
> migration/migration.c | 21 +++++++++++++++++++++
> migration/migration.h | 3 +++
> migration/savevm.c | 36 ++++++++++++++++++++++++++++++++++++
> migration/savevm.h | 2 ++
> qapi/migration.json | 5 ++++-
> 6 files changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index b9ee563aa4..ac4f46a67d 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -186,6 +186,8 @@ struct VMStateDescription {
> int (*pre_save)(void *opaque);
> int (*post_save)(void *opaque);
> bool (*needed)(void *opaque);
> + bool (*dev_unplug_pending)(void *opaque);
> +
> const VMStateField *fields;
> const VMStateDescription **subsections;
> };
> diff --git a/migration/migration.c b/migration/migration.c
> index 3febd0f8f3..51764f2565 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -52,6 +52,7 @@
> #include "hw/qdev-properties.h"
> #include "monitor/monitor.h"
> #include "net/announce.h"
> +#include "qemu/queue.h"
>
> #define MAX_THROTTLE (32 << 20) /* Migration transfer speed throttling */
>
> @@ -819,6 +820,7 @@ bool migration_is_setup_or_active(int state)
> case MIGRATION_STATUS_SETUP:
> case MIGRATION_STATUS_PRE_SWITCHOVER:
> case MIGRATION_STATUS_DEVICE:
> + case MIGRATION_STATUS_WAIT_UNPLUG:
> return true;
>
> default:
> @@ -954,6 +956,9 @@ static void fill_source_migration_info(MigrationInfo *info)
> case MIGRATION_STATUS_CANCELLED:
> info->has_status = true;
> break;
> + case MIGRATION_STATUS_WAIT_UNPLUG:
> + info->has_status = true;
> + break;
> }
> info->status = s->state;
> }
> @@ -1694,6 +1699,7 @@ bool migration_is_idle(void)
> case MIGRATION_STATUS_COLO:
> case MIGRATION_STATUS_PRE_SWITCHOVER:
> case MIGRATION_STATUS_DEVICE:
> + case MIGRATION_STATUS_WAIT_UNPLUG:
> return false;
> case MIGRATION_STATUS__MAX:
> g_assert_not_reached();
> @@ -3264,6 +3270,19 @@ static void *migration_thread(void *opaque)
>
> qemu_savevm_state_setup(s->to_dst_file);
>
> + if (qemu_savevm_nr_failover_devices()) {
> + migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> + MIGRATION_STATUS_WAIT_UNPLUG);
> +
> + while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
> + !qemu_savevm_state_guest_unplug_pending()) {
> + qemu_sem_timedwait(&s->wait_unplug_sem, 250);
> + }
> +
> + migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG,
> + MIGRATION_STATUS_ACTIVE);
> + }
> +
> s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> MIGRATION_STATUS_ACTIVE);
> @@ -3511,6 +3530,7 @@ static void migration_instance_finalize(Object *obj)
> qemu_mutex_destroy(&ms->qemu_file_lock);
> g_free(params->tls_hostname);
> g_free(params->tls_creds);
> + qemu_sem_destroy(&ms->wait_unplug_sem);
> qemu_sem_destroy(&ms->rate_limit_sem);
> qemu_sem_destroy(&ms->pause_sem);
> qemu_sem_destroy(&ms->postcopy_pause_sem);
> @@ -3556,6 +3576,7 @@ static void migration_instance_init(Object *obj)
> qemu_sem_init(&ms->postcopy_pause_rp_sem, 0);
> qemu_sem_init(&ms->rp_state.rp_sem, 0);
> qemu_sem_init(&ms->rate_limit_sem, 0);
> + qemu_sem_init(&ms->wait_unplug_sem, 0);
> qemu_mutex_init(&ms->qemu_file_lock);
> }
>
> diff --git a/migration/migration.h b/migration/migration.h
> index 4f2fe193dc..79b3dda146 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -206,6 +206,9 @@ struct MigrationState
> /* Flag set once the migration thread called bdrv_inactivate_all */
> bool block_inactive;
>
> + /* Migration is waiting for guest to unplug device */
> + QemuSemaphore wait_unplug_sem;
> +
> /* Migration is paused due to pause-before-switchover */
> QemuSemaphore pause_sem;
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 8d95e261f6..0f18dea49e 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1113,6 +1113,42 @@ void qemu_savevm_state_header(QEMUFile *f)
> }
> }
>
> +int qemu_savevm_nr_failover_devices(void)
> +{
> + SaveStateEntry *se;
> + int n = 0;
> +
> + QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> + if (se->vmsd && se->vmsd->dev_unplug_pending) {
> + n++;
> + }
> + }
> +
> + return n;
> +}
> +
> +bool qemu_savevm_state_guest_unplug_pending(void)
> +{
> + int nr_failover_devs;
> + SaveStateEntry *se;
> + bool ret = false;
> + int n = 0;
> +
> + nr_failover_devs = qemu_savevm_nr_failover_devices();
> +
> + QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> + if (!se->vmsd || !se->vmsd->dev_unplug_pending) {
> + continue;
> + }
> + ret = se->vmsd->dev_unplug_pending(se->opaque);
> + if (!ret) {
> + n++;
> + }
> + }
> +
> + return n == nr_failover_devs;
I was expecting != I think? If all the devices say
they've got one pending then doesn't n==nr_failover_devs and
it returns true? But then what happens if only one has one pending?
Dave
> +}
> +
> void qemu_savevm_state_setup(QEMUFile *f)
> {
> SaveStateEntry *se;
> diff --git a/migration/savevm.h b/migration/savevm.h
> index 51a4b9caa8..c42b9c80ee 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -31,6 +31,8 @@
>
> bool qemu_savevm_state_blocked(Error **errp);
> void qemu_savevm_state_setup(QEMUFile *f);
> +int qemu_savevm_nr_failover_devices(void);
> +bool qemu_savevm_state_guest_unplug_pending(void);
> int qemu_savevm_state_resume_prepare(MigrationState *s);
> void qemu_savevm_state_header(QEMUFile *f);
> int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
> diff --git a/qapi/migration.json b/qapi/migration.json
> index e9e7a97c03..b7348d0c8b 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -133,6 +133,9 @@
> # @device: During device serialisation when pause-before-switchover is enabled
> # (since 2.11)
> #
> +# @wait-unplug: wait for device unplug request by guest OS to be completed.
> +# (since 4.2)
> +#
> # Since: 2.3
> #
> ##
> @@ -140,7 +143,7 @@
> 'data': [ 'none', 'setup', 'cancelling', 'cancelled',
> 'active', 'postcopy-active', 'postcopy-paused',
> 'postcopy-recover', 'completed', 'failed', 'colo',
> - 'pre-switchover', 'device' ] }
> + 'pre-switchover', 'device', 'wait-unplug' ] }
>
> ##
> # @MigrationInfo:
> --
> 2.21.0
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
next prev parent reply other threads:[~2019-10-29 2:57 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-25 12:19 [PATCH v6 0/11] add failover feature for assigned network devices Jens Freimann
2019-10-25 12:19 ` [PATCH v6 01/11] qdev/qbus: add hidden device support Jens Freimann
2019-10-25 17:34 ` Dr. David Alan Gilbert
2019-10-25 12:19 ` [PATCH v6 02/11] pci: add option for net failover Jens Freimann
2019-10-25 12:19 ` [PATCH v6 03/11] pci: mark devices partially unplugged Jens Freimann
2019-10-25 12:19 ` [PATCH v6 04/11] pci: mark device having guest unplug request pending Jens Freimann
2019-10-25 12:19 ` [PATCH v6 05/11] qapi: add unplug primary event Jens Freimann
2019-10-25 12:19 ` [PATCH v6 06/11] qapi: add failover negotiated event Jens Freimann
2019-10-25 14:03 ` Markus Armbruster
2019-10-25 17:39 ` Jens Freimann
2019-10-29 12:13 ` Markus Armbruster
2019-10-29 22:41 ` Michael S. Tsirkin
2019-10-30 10:09 ` Jens Freimann
2019-10-25 12:19 ` [PATCH v6 07/11] migration: allow unplug during migration for failover devices Jens Freimann
2019-10-25 12:19 ` [PATCH v6 08/11] migration: add new migration state wait-unplug Jens Freimann
2019-10-29 2:56 ` Dr. David Alan Gilbert [this message]
2019-10-29 5:51 ` Jens Freimann
2019-10-29 7:09 ` Dr. David Alan Gilbert
2019-10-25 12:19 ` [PATCH v6 09/11] libqos: tolerate wait-unplug migration state Jens Freimann
2019-10-25 12:19 ` [PATCH v6 10/11] net/virtio: add failover support Jens Freimann
2019-10-25 12:19 ` [PATCH v6 11/11] vfio: unplug failover primary device before migration Jens Freimann
2019-10-25 15:23 ` [PATCH v6 0/11] add failover feature for assigned network devices Roman Kagan
2019-10-28 10:27 ` Jens Freimann
2019-10-28 15:58 ` Michael S. Tsirkin
2019-10-29 11:18 ` Jens Freimann
2019-10-29 22:49 ` Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191029025650.GE2508@work-vm \
--to=dgilbert@redhat.com \
--cc=aadam@redhat.com \
--cc=ailan@redhat.com \
--cc=alex.williamson@redhat.com \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=ehabkost@redhat.com \
--cc=jasowang@redhat.com \
--cc=jfreimann@redhat.com \
--cc=laine@redhat.com \
--cc=mst@redhat.com \
--cc=parav@mellanox.com \
--cc=pkrempa@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).