| Message ID | 20190517125820.2885-2-jfreimann@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | add failover feature for assigned networkdevices | expand |
* Jens Freimann (jfreimann@redhat.com) wrote: > In "b06424de62 migration: Disable hotplug/unplug during migration" we > added a check to disable unplug for all devices until we have figured > out what works. For failover primary devices qdev_unplug() is called > from the migration handler, i.e. during migration. > > This patch adds a flag to DeviceState which is set to false for all > devices and makes an exception for vfio-pci devices that are also > primary devices in a failover pair. > > Signed-off-by: Jens Freimann <jfreimann@redhat.com> So I think this is safe in your case, because you trigger the unplug right at the start of migration during setup and plug after failure; however it's not generally safe - I can't unplug a device while the migration is actually in progress. Dave > --- > hw/core/qdev.c | 1 + > include/hw/qdev-core.h | 1 + > qdev-monitor.c | 2 +- > 3 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index f9b6efe509..98cdaa6bf7 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -954,6 +954,7 @@ static void device_initfn(Object *obj) > > dev->instance_id_alias = -1; > dev->realized = false; > + dev->allow_unplug_during_migration = false; > > object_property_add_bool(obj, "realized", > device_get_realized, device_set_realized, NULL); > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 33ed3b8dde..5437395779 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -146,6 +146,7 @@ struct DeviceState { > bool pending_deleted_event; > QemuOpts *opts; > int hotplugged; > + bool allow_unplug_during_migration; > BusState *parent_bus; > QLIST_HEAD(, NamedGPIOList) gpios; > QLIST_HEAD(, BusState) child_bus; > diff --git a/qdev-monitor.c b/qdev-monitor.c > index 373b9ad445..9cce8b93c2 100644 > --- a/qdev-monitor.c > +++ b/qdev-monitor.c > @@ -867,7 +867,7 @@ void qdev_unplug(DeviceState *dev, Error **errp) > return; > } > > - if (!migration_is_idle()) { > + if (!migration_is_idle() && !dev->allow_unplug_during_migration) { > error_setg(errp, "device_del not allowed while migrating"); > return; > } > -- > 2.21.0 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Tue, May 21, 2019 at 10:33:36AM +0100, Dr. David Alan Gilbert wrote: > * Jens Freimann (jfreimann@redhat.com) wrote: > > In "b06424de62 migration: Disable hotplug/unplug during migration" we > > added a check to disable unplug for all devices until we have figured > > out what works. For failover primary devices qdev_unplug() is called > > from the migration handler, i.e. during migration. > > > > This patch adds a flag to DeviceState which is set to false for all > > devices and makes an exception for vfio-pci devices that are also > > primary devices in a failover pair. > > > > Signed-off-by: Jens Freimann <jfreimann@redhat.com> > > So I think this is safe in your case, because you trigger the unplug > right at the start of migration during setup and plug after failure; > however it's not generally safe - I can't unplug a device while the > migration is actually in progress. Libvirt will also block any attempt to hotplug/unplug device during migration. Regards, Daniel
On Tue, May 21, 2019 at 10:33:36AM +0100, Dr. David Alan Gilbert wrote: >* Jens Freimann (jfreimann@redhat.com) wrote: >> In "b06424de62 migration: Disable hotplug/unplug during migration" we >> added a check to disable unplug for all devices until we have figured >> out what works. For failover primary devices qdev_unplug() is called >> from the migration handler, i.e. during migration. >> >> This patch adds a flag to DeviceState which is set to false for all >> devices and makes an exception for vfio-pci devices that are also >> primary devices in a failover pair. >> >> Signed-off-by: Jens Freimann <jfreimann@redhat.com> > >So I think this is safe in your case, because you trigger the unplug >right at the start of migration during setup and plug after failure; >however it's not generally safe - I can't unplug a device while the >migration is actually in progress. I tried to limit it to only allow it in failover case. You're saying it's missing something and not strict enough? I could allow it only during migration setup. I guess we'll need a similar exception for failover in libvirt. regards, Jens
* Jens Freimann (jfreimann@redhat.com) wrote: > On Tue, May 21, 2019 at 10:33:36AM +0100, Dr. David Alan Gilbert wrote: > > * Jens Freimann (jfreimann@redhat.com) wrote: > > > In "b06424de62 migration: Disable hotplug/unplug during migration" we > > > added a check to disable unplug for all devices until we have figured > > > out what works. For failover primary devices qdev_unplug() is called > > > from the migration handler, i.e. during migration. > > > > > > This patch adds a flag to DeviceState which is set to false for all > > > devices and makes an exception for vfio-pci devices that are also > > > primary devices in a failover pair. > > > > > > Signed-off-by: Jens Freimann <jfreimann@redhat.com> > > > > So I think this is safe in your case, because you trigger the unplug > > right at the start of migration during setup and plug after failure; > > however it's not generally safe - I can't unplug a device while the > > migration is actually in progress. > > I tried to limit it to only allow it in failover case. You're saying > it's missing something and not strict enough? I could allow it only > during migration setup. I guess we'll need a similar exception for > failover in libvirt. I might be wrong, but I think with your patch I could hot unplug your device part way through migration; where as I think you only care about it doing it at a very specific point during setup. (I still would prefer the hotplug to be done outside qemu, but still that's separate). Dave > regards, > Jens -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index f9b6efe509..98cdaa6bf7 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -954,6 +954,7 @@ static void device_initfn(Object *obj) dev->instance_id_alias = -1; dev->realized = false; + dev->allow_unplug_during_migration = false; object_property_add_bool(obj, "realized", device_get_realized, device_set_realized, NULL); diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 33ed3b8dde..5437395779 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -146,6 +146,7 @@ struct DeviceState { bool pending_deleted_event; QemuOpts *opts; int hotplugged; + bool allow_unplug_during_migration; BusState *parent_bus; QLIST_HEAD(, NamedGPIOList) gpios; QLIST_HEAD(, BusState) child_bus; diff --git a/qdev-monitor.c b/qdev-monitor.c index 373b9ad445..9cce8b93c2 100644 --- a/qdev-monitor.c +++ b/qdev-monitor.c @@ -867,7 +867,7 @@ void qdev_unplug(DeviceState *dev, Error **errp) return; } - if (!migration_is_idle()) { + if (!migration_is_idle() && !dev->allow_unplug_during_migration) { error_setg(errp, "device_del not allowed while migrating"); return; }
In "b06424de62 migration: Disable hotplug/unplug during migration" we added a check to disable unplug for all devices until we have figured out what works. For failover primary devices qdev_unplug() is called from the migration handler, i.e. during migration. This patch adds a flag to DeviceState which is set to false for all devices and makes an exception for vfio-pci devices that are also primary devices in a failover pair. Signed-off-by: Jens Freimann <jfreimann@redhat.com> --- hw/core/qdev.c | 1 + include/hw/qdev-core.h | 1 + qdev-monitor.c | 2 +- 3 files changed, 3 insertions(+), 1 deletion(-)