[Qemu-devel] [PATCH v2 0/5] virtio-9p: hotplug and migration support

[Qemu-devel] [PATCH v2 0/5] virtio-9p: hotplug and migration support

[Greg Kurz]

Hi,

We already have a blocker to prevent migration of an active virtio-9p device.
But in fact, there is no migration support at all for 9p, even if the device
is considered to be quiescent (when the VirtFS share is not mounted): migration
succeeds but the device is lost in the restarted guest.
Hotunplug of a virtio-9p device is not supported either (no unrealize handler)
and leads to a QEMU crash on the source node, if one unplugs and migrates.

This series tries to fix that and brings hotplug and migration support of
*quiescent* virtio-9p devices.

The most notable change since my previous post is the introduction of an unplug
blocker (patch 2/5 and 3/5). I also reworked the series so that some fixes
appear in more appropriate patches (see individual changelogs).

Please comment.

--
Greg

---

Greg Kurz (5):
      virtio-9p-coth: fix init function
      qdev: add the HotUnpluggable handler
      virtio-9p: block hot-unplug when device is active
      virtio-9p: add unrealize handler
      virtio-9p: add savem handlers


 hw/9pfs/virtio-9p-coth.c   |   22 ++++++++++++++++++----
 hw/9pfs/virtio-9p-coth.h   |    2 ++
 hw/9pfs/virtio-9p-device.c |   24 ++++++++++++++++++++++++
 hw/9pfs/virtio-9p.c        |   14 ++++++++++++++
 hw/9pfs/virtio-9p.h        |    2 ++
 hw/core/qdev.c             |    4 ++++
 hw/s390x/virtio-ccw.c      |    8 ++++++++
 hw/virtio/virtio-pci.c     |    8 ++++++++
 include/hw/qdev-core.h     |    4 ++++
 9 files changed, 84 insertions(+), 4 deletions(-)

[Qemu-devel] [PATCH v2 1/5] virtio-9p-coth: fix init function

[Greg Kurz]

The v9fs thread pool is a singleton, shared by all virtio-9p devices.
The current code leaks underlying glib pointers each time a new virtio-9p
device gets realized. Let's fix that !

While we're here, we also fix the trivial error path when memory allocation
is failing.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
v2:
- fix memory leak
- moved introduction of v9fs_release_worker_threads() to patch 4/5
- renamed patch title
---
 hw/9pfs/virtio-9p-coth.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/virtio-9p-coth.c b/hw/9pfs/virtio-9p-coth.c
index 8185c533c013..1d832ede1ebf 100644
--- a/hw/9pfs/virtio-9p-coth.c
+++ b/hw/9pfs/virtio-9p-coth.c
@@ -55,6 +55,10 @@ int v9fs_init_worker_threads(void)
     V9fsThPool *p = &v9fs_pool;
     sigset_t set, oldset;
 
+    if (p->pool) {
+        return 0;
+    }
+
     sigfillset(&set);
     /* Leave signal handling to the iothread.  */
     pthread_sigmask(SIG_SETMASK, &set, &oldset);
@@ -66,10 +70,7 @@ int v9fs_init_worker_threads(void)
     }
     p->completed = g_async_queue_new();
     if (!p->completed) {
-        /*
-         * We are going to terminate.
-         * So don't worry about cleanup
-         */
+        g_thread_pool_free(p->pool, TRUE, TRUE);
         ret = -1;
         goto err_out;
     }

[Qemu-devel] [PATCH v2 2/5] qdev: add the HotUnpluggable handler

[Greg Kurz]

This handler allows to ask a device instance if it can be hot-unplugged. It
is to be defined in device classes where hot-unpluggability depends on the
device state (for example, virtio-9p devices cannot be unplugged if the 9p
share is mounted in the guest).

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/core/qdev.c         |    4 ++++
 include/hw/qdev-core.h |    4 ++++
 2 files changed, 8 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 4ab04aa31e78..2b2339c7c6ad 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -287,6 +287,10 @@ void qdev_unplug(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (dc->unpluggable && !dc->unpluggable(dev, errp)) {
+        return;
+    }
+
     qdev_hot_removed = true;
 
     hotplug_ctrl = qdev_get_hotplug_handler(dev);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 038b54d94b27..5df0db1a5b68 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -38,6 +38,7 @@ typedef void (*DeviceRealize)(DeviceState *dev, Error **errp);
 typedef void (*DeviceUnrealize)(DeviceState *dev, Error **errp);
 typedef void (*BusRealize)(BusState *bus, Error **errp);
 typedef void (*BusUnrealize)(BusState *bus, Error **errp);
+typedef bool (*HotUnpluggable)(DeviceState *dev, Error **errp);
 
 struct VMStateDescription;
 
@@ -48,6 +49,8 @@ struct VMStateDescription;
  * property is changed to %true. The default invokes @init if not %NULL.
  * @unrealize: Callback function invoked when the #DeviceState:realized
  * property is changed to %false.
+ * @unpluggable: Callback function invoked by qdev_unplug(). Return %false
+ * to block hotunplug.
  * @init: Callback function invoked when the #DeviceState::realized property
  * is changed to %true. Deprecated, new types inheriting directly from
  * TYPE_DEVICE should use @realize instead, new leaf types should consult
@@ -120,6 +123,7 @@ typedef struct DeviceClass {
     void (*reset)(DeviceState *dev);
     DeviceRealize realize;
     DeviceUnrealize unrealize;
+    HotUnpluggable unpluggable;
 
     /* device state */
     const struct VMStateDescription *vmsd;

Re: [Qemu-devel] [PATCH v2 2/5] qdev: add the HotUnpluggable handler

[Cornelia Huck]

On Mon, 12 Oct 2015 11:00:13 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> This handler allows to ask a device instance if it can be hot-unplugged. It
> is to be defined in device classes where hot-unpluggability depends on the
> device state (for example, virtio-9p devices cannot be unplugged if the 9p
> share is mounted in the guest).
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/core/qdev.c         |    4 ++++
>  include/hw/qdev-core.h |    4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 4ab04aa31e78..2b2339c7c6ad 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -287,6 +287,10 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>          return;
>      }
> 
> +    if (dc->unpluggable && !dc->unpluggable(dev, errp)) {

I think I'd prefer something like "unplug_is_blocked" or so. Makes it
more obvious that this is something that is rather the exception. 

> +        return;
> +    }
> +
>      qdev_hot_removed = true;
> 
>      hotplug_ctrl = qdev_get_hotplug_handler(dev);

Re: [Qemu-devel] [PATCH v2 2/5] qdev: add the HotUnpluggable handler

[Greg Kurz]

On Thu, 15 Oct 2015 17:14:36 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Mon, 12 Oct 2015 11:00:13 +0200
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > This handler allows to ask a device instance if it can be hot-unplugged. It
> > is to be defined in device classes where hot-unpluggability depends on the
> > device state (for example, virtio-9p devices cannot be unplugged if the 9p
> > share is mounted in the guest).
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  hw/core/qdev.c         |    4 ++++
> >  include/hw/qdev-core.h |    4 ++++
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 4ab04aa31e78..2b2339c7c6ad 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -287,6 +287,10 @@ void qdev_unplug(DeviceState *dev, Error **errp)
> >          return;
> >      }
> > 
> > +    if (dc->unpluggable && !dc->unpluggable(dev, errp)) {
> 
> I think I'd prefer something like "unplug_is_blocked" or so. Makes it
> more obvious that this is something that is rather the exception. 
> 

It makes sense indeed. I'll do that for next version.

> > +        return;
> > +    }
> > +
> >      qdev_hot_removed = true;
> > 
> >      hotplug_ctrl = qdev_get_hotplug_handler(dev);

[Qemu-devel] [PATCH v2 3/5] virtio-9p: block hot-unplug when device is active

[Greg Kurz]

Hot-unplug of an active virtio-9p device is not currently supported. Until
we have that, let's block hot-unplugging when the 9p share is mounted in
the guest.

This patch implements a hot-unplug blocker feature like we already have for
migration. Both virtio-9p-pci and virtio-9p-ccw were adapted accordingly.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p.c    |   14 ++++++++++++++
 hw/9pfs/virtio-9p.h    |    2 ++
 hw/s390x/virtio-ccw.c  |    8 ++++++++
 hw/virtio/virtio-pci.c |    8 ++++++++
 4 files changed, 32 insertions(+)

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index f972731f5a8d..398bdbc8d0e7 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -345,6 +345,7 @@ static int put_fid(V9fsPDU *pdu, V9fsFidState *fidp)
                 migrate_del_blocker(pdu->s->migration_blocker);
                 error_free(pdu->s->migration_blocker);
                 pdu->s->migration_blocker = NULL;
+                pdu->s->unplug_is_blocked = false;
             }
         }
         return free_fid(pdu, fidp);
@@ -991,6 +992,7 @@ static void v9fs_attach(void *opaque)
                    "Migration is disabled when VirtFS export path '%s' is mounted in the guest using mount_tag '%s'",
                    s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag);
         migrate_add_blocker(s->migration_blocker);
+        s->unplug_is_blocked = true;
     }
 out:
     put_fid(pdu, fidp);
@@ -3288,6 +3290,18 @@ void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
     free_pdu(s, pdu);
 }
 
+bool v9fs_unpluggable(V9fsState *s, Error **errp)
+{
+    if (s->unplug_is_blocked) {
+        error_setg(errp,
+                   "Unplug is disabled when VirtFS export path '%s' is mounted"
+                   " in the guest using mount_tag '%s'",
+                   s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag);
+        return false;
+    }
+    return true;
+}
+
 static void __attribute__((__constructor__)) virtio_9p_set_fd_limit(void)
 {
     struct rlimit rlim;
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index 2e7d48857083..ddc1cbf210ca 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -218,6 +218,7 @@ typedef struct V9fsState
     CoRwlock rename_lock;
     int32_t root_fid;
     Error *migration_blocker;
+    bool unplug_is_blocked;
     V9fsConf fsconf;
 } V9fsState;
 
@@ -381,6 +382,7 @@ extern void v9fs_path_free(V9fsPath *path);
 extern void v9fs_path_copy(V9fsPath *lhs, V9fsPath *rhs);
 extern int v9fs_name_to_path(V9fsState *s, V9fsPath *dirpath,
                              const char *name, V9fsPath *path);
+extern bool v9fs_unpluggable(V9fsState *s, Error **errp);
 
 #define pdu_marshal(pdu, offset, fmt, args...)  \
     v9fs_marshal(pdu->elem.in_sg, pdu->elem.in_num, offset, 1, fmt, ##args)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index fb103b78ac28..d43d8c4d2457 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1924,6 +1924,13 @@ static void virtio_ccw_9p_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     }
 }
 
+static bool virtio_ccw_9p_unpluggable(DeviceState *dev, Error **errp)
+{
+    V9fsCCWState *v9fs_ccw_dev = VIRTIO_9P_CCW(dev);
+
+    return v9fs_unpluggable(&v9fs_ccw_dev->vdev, errp);
+}
+
 static void virtio_ccw_9p_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1931,6 +1938,7 @@ static void virtio_ccw_9p_class_init(ObjectClass *klass, void *data)
 
     k->exit = virtio_ccw_exit;
     k->realize = virtio_ccw_9p_realize;
+    dc->unpluggable = virtio_ccw_9p_unpluggable;
     dc->reset = virtio_ccw_reset;
     dc->props = virtio_ccw_9p_properties;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 6703806f837b..e3839443c71c 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1015,6 +1015,13 @@ static void virtio_9p_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     object_property_set_bool(OBJECT(vdev), true, "realized", errp);
 }
 
+static bool virtio_9p_pci_unpluggable(DeviceState *dev, Error **errp)
+{
+    V9fsPCIState *v9fs_pci_dev = VIRTIO_9P_PCI(dev);
+
+    return v9fs_unpluggable(&v9fs_pci_dev->vdev, errp);
+}
+
 static Property virtio_9p_pci_properties[] = {
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
                     VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
@@ -1035,6 +1042,7 @@ static void virtio_9p_pci_class_init(ObjectClass *klass, void *data)
     pcidev_k->class_id = 0x2;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->props = virtio_9p_pci_properties;
+    dc->unpluggable = virtio_9p_pci_unpluggable;
 }
 
 static void virtio_9p_pci_instance_init(Object *obj)

[Qemu-devel] [PATCH v2 4/5] virtio-9p: add unrealize handler

[Greg Kurz]

This patch allows to hot-unplug a quiescent virtio-9p device, and free its
allocated resources. A refcount is added to the v9fs thread pool, so that
its resources are freed as well when the last virtio-9p device is unplugged.

Note that we have an unplug blocker which prevents this code to be reached
if the 9p share is mounted in the guest. No need to bother about in-flight
I/O requests in this case.

This patch fixes a QEMU crash on the source node if the user device_add
a virtio-9p device and then migrate.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
V2:
- now introduces v9fs_release_worker_threads() (was in patch 1)
- adds a refcount to the v9fs thread pool
---
 hw/9pfs/virtio-9p-coth.c   |   15 ++++++++++++++-
 hw/9pfs/virtio-9p-coth.h   |    2 ++
 hw/9pfs/virtio-9p-device.c |   12 ++++++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/virtio-9p-coth.c b/hw/9pfs/virtio-9p-coth.c
index 1d832ede1ebf..27ccc66c91d8 100644
--- a/hw/9pfs/virtio-9p-coth.c
+++ b/hw/9pfs/virtio-9p-coth.c
@@ -55,7 +55,7 @@ int v9fs_init_worker_threads(void)
     V9fsThPool *p = &v9fs_pool;
     sigset_t set, oldset;
 
-    if (p->pool) {
+    if (p->refcount++) {
         return 0;
     }
 
@@ -81,3 +81,16 @@ err_out:
     pthread_sigmask(SIG_SETMASK, &oldset, NULL);
     return ret;
 }
+
+void v9fs_release_worker_threads(void)
+{
+    V9fsThPool *p = &v9fs_pool;
+
+    if (--p->refcount) {
+        return;
+    }
+
+    g_thread_pool_free(p->pool, TRUE, TRUE);
+    g_async_queue_unref(p->completed);
+    event_notifier_set_handler(&p->e, NULL);
+}
diff --git a/hw/9pfs/virtio-9p-coth.h b/hw/9pfs/virtio-9p-coth.h
index 4f51b250d1d4..2a2617e670a5 100644
--- a/hw/9pfs/virtio-9p-coth.h
+++ b/hw/9pfs/virtio-9p-coth.h
@@ -25,6 +25,7 @@ typedef struct V9fsThPool {
 
     GThreadPool *pool;
     GAsyncQueue *completed;
+    unsigned refcount;
 } V9fsThPool;
 
 /*
@@ -56,6 +57,7 @@ typedef struct V9fsThPool {
 
 extern void co_run_in_worker_bh(void *);
 extern int v9fs_init_worker_threads(void);
+extern void v9fs_release_worker_threads(void);
 extern int v9fs_co_readlink(V9fsPDU *, V9fsPath *, V9fsString *);
 extern int v9fs_co_readdir_r(V9fsPDU *, V9fsFidState *,
                            struct dirent *, struct dirent **result);
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 93a407c45926..ed133c40493a 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -138,6 +138,17 @@ out:
     v9fs_path_free(&path);
 }
 
+static void virtio_9p_device_unrealize(DeviceState *dev, Error **errp)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    V9fsState *s = VIRTIO_9P(dev);
+
+    v9fs_release_worker_threads();
+    g_free(s->ctx.fs_root);
+    g_free(s->tag);
+    virtio_cleanup(vdev);
+}
+
 /* virtio-9p device */
 
 static Property virtio_9p_properties[] = {
@@ -154,6 +165,7 @@ static void virtio_9p_class_init(ObjectClass *klass, void *data)
     dc->props = virtio_9p_properties;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     vdc->realize = virtio_9p_device_realize;
+    vdc->unrealize = virtio_9p_device_unrealize;
     vdc->get_features = virtio_9p_get_features;
     vdc->get_config = virtio_9p_get_config;
 }

[Qemu-devel] [PATCH v2 5/5] virtio-9p: add savem handlers

[Greg Kurz]

We don't support migration of mounted 9p shares. This is handled by a
migration blocker.

One would expect, however, to be able to migrate if the share is unmounted.
Unfortunately virtio-9p-device does not register savevm handlers at all !
Migration succeeds and leaves the guest with a dangling device...

This patch simply registers migration handlers for virtio-9p-device. Whether
migration is possible or not still depends on the migration blocker.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-device.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index ed133c40493a..bd7f10a0a902 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -43,6 +43,16 @@ static void virtio_9p_get_config(VirtIODevice *vdev, uint8_t *config)
     g_free(cfg);
 }
 
+static void virtio_9p_save(QEMUFile *f, void *opaque)
+{
+    virtio_save(VIRTIO_DEVICE(opaque), f);
+}
+
+static int virtio_9p_load(QEMUFile *f, void *opaque, int version_id)
+{
+    return virtio_load(VIRTIO_DEVICE(opaque), f, version_id);
+}
+
 static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -130,6 +140,7 @@ static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
     }
     v9fs_path_free(&path);
 
+    register_savevm(dev, "virtio-9p", -1, 1, virtio_9p_save, virtio_9p_load, s);
     return;
 out:
     g_free(s->ctx.fs_root);
@@ -146,6 +157,7 @@ static void virtio_9p_device_unrealize(DeviceState *dev, Error **errp)
     v9fs_release_worker_threads();
     g_free(s->ctx.fs_root);
     g_free(s->tag);
+    unregister_savevm(dev, "virtio-9p", s);
     virtio_cleanup(vdev);
 }