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

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

[Greg Kurz]

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.

v2->v3:
 - renamed QDEV handler @unpluggable to @unplug_is_blocked (patches 2/5
   and 3/5)

v1->v2:
 - introduced unplug blocker (patches 2/5 and 3/5)
 - moved fixes to separate patches (see individual changelogs)

---

Greg Kurz (5):
      virtio-9p-coth: fix init function
      qdev: add the @unplug_is_blocked 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 v3 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>
---
 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 v3 2/5] qdev: add the @unplug_is_blocked 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..b37a3801117a 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->unplug_is_blocked && !dc->unplug_is_blocked(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 8057aedaa6c0..a65d2be6f39b 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 (*HotUnplugBlocked)(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.
+ * @unplug_is_blocked: 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
@@ -133,6 +136,7 @@ typedef struct DeviceClass {
     void (*reset)(DeviceState *dev);
     DeviceRealize realize;
     DeviceUnrealize unrealize;
+    HotUnplugBlocked unplug_is_blocked;
 
     /* device state */
     const struct VMStateDescription *vmsd;

[Qemu-devel] [PATCH v3 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..bbf39ed33983 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_unplug_is_blocked(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..8d4cbed2a5c4 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_unplug_is_blocked(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..5c13072ec96e 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_unplug_is_blocked(DeviceState *dev, Error **errp)
+{
+    V9fsCCWState *v9fs_ccw_dev = VIRTIO_9P_CCW(dev);
+
+    return v9fs_unplug_is_blocked(&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->unplug_is_blocked = virtio_ccw_9p_unplug_is_blocked;
     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 e5c406d1d255..bf0d516528ee 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_unplug_is_blocked(DeviceState *dev, Error **errp)
+{
+    V9fsPCIState *v9fs_pci_dev = VIRTIO_9P_PCI(dev);
+
+    return v9fs_unplug_is_blocked(&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->unplug_is_blocked = virtio_9p_pci_unplug_is_blocked;
 }
 
 static void virtio_9p_pci_instance_init(Object *obj)

[Qemu-devel] [PATCH v3 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>
---
 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 v3 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);
 }
 

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

[Michael S. Tsirkin]

On Tue, Oct 20, 2015 at 11:17:00AM +0200, Greg Kurz wrote:
> 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>

I'm not sure that's right.
This isn't what we do for other devices.
Why doesn't unplug request cause filesystem to be unmounted?
Isn't this just a guest bug?
And if yes, why do we need a blocker in qemu?

> ---
>  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..bbf39ed33983 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_unplug_is_blocked(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..8d4cbed2a5c4 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_unplug_is_blocked(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..5c13072ec96e 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_unplug_is_blocked(DeviceState *dev, Error **errp)
> +{
> +    V9fsCCWState *v9fs_ccw_dev = VIRTIO_9P_CCW(dev);
> +
> +    return v9fs_unplug_is_blocked(&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->unplug_is_blocked = virtio_ccw_9p_unplug_is_blocked;
>      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 e5c406d1d255..bf0d516528ee 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_unplug_is_blocked(DeviceState *dev, Error **errp)
> +{
> +    V9fsPCIState *v9fs_pci_dev = VIRTIO_9P_PCI(dev);
> +
> +    return v9fs_unplug_is_blocked(&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->unplug_is_blocked = virtio_9p_pci_unplug_is_blocked;
>  }
>  
>  static void virtio_9p_pci_instance_init(Object *obj)

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

[Michael S. Tsirkin]

On Tue, Oct 20, 2015 at 11:16:40AM +0200, Greg Kurz wrote:
> 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.
> 
> v2->v3:
>  - renamed QDEV handler @unpluggable to @unplug_is_blocked (patches 2/5
>    and 3/5)
> 
> v1->v2:
>  - introduced unplug blocker (patches 2/5 and 3/5)
>  - moved fixes to separate patches (see individual changelogs)

I have some doubts about how hotunplug is handled, but migration
looks ok.
Is there a dependency, or can I just pick savevm things meanwhile?

> ---
> 
> Greg Kurz (5):
>       virtio-9p-coth: fix init function
>       qdev: add the @unplug_is_blocked 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(-)

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

[Greg Kurz]

On Tue, 20 Oct 2015 15:42:03 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Oct 20, 2015 at 11:17:00AM +0200, Greg Kurz wrote:
> > 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>
> 
> I'm not sure that's right.
> This isn't what we do for other devices.

Indeed.

> Why doesn't unplug request cause filesystem to be unmounted?

The 9p/trans_virtio driver does not do the necessary steps to
make that happen. It just waits for the users to close and
prints a repeating message to say so.

> Isn't this just a guest bug?

Yes it is.

> And if yes, why do we need a blocker in qemu?
> 

Without blocker, the only hint that we tried to unplug an active device is
either "9pnet_virtio virtioX: p9_virtio_remove: waiting for device in use"
being printed every 10 sec in the guest console, either a crash if the
guest driver does not have this commit:

commit 8051a2a518fcf3827a143470083ad6008697ff17
Author: Michael S. Tsirkin <mst@redhat.com>
Date:   Thu Mar 12 11:53:41 2015 +1030

    9p/trans_virtio: fix hot-unplug

The blocker allows to return a more comprehensive error directly to the
caller. IMHO it is friendlier to the user than parsing dmesg, and it
makes sense to have that until all guests are fixed.

> > ---
> >  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..bbf39ed33983 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_unplug_is_blocked(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..8d4cbed2a5c4 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_unplug_is_blocked(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..5c13072ec96e 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_unplug_is_blocked(DeviceState *dev, Error **errp)
> > +{
> > +    V9fsCCWState *v9fs_ccw_dev = VIRTIO_9P_CCW(dev);
> > +
> > +    return v9fs_unplug_is_blocked(&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->unplug_is_blocked = virtio_ccw_9p_unplug_is_blocked;
> >      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 e5c406d1d255..bf0d516528ee 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_unplug_is_blocked(DeviceState *dev, Error **errp)
> > +{
> > +    V9fsPCIState *v9fs_pci_dev = VIRTIO_9P_PCI(dev);
> > +
> > +    return v9fs_unplug_is_blocked(&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->unplug_is_blocked = virtio_9p_pci_unplug_is_blocked;
> >  }
> >  
> >  static void virtio_9p_pci_instance_init(Object *obj)
> 

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

[Greg Kurz]

On Tue, 20 Oct 2015 15:53:08 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Oct 20, 2015 at 11:16:40AM +0200, Greg Kurz wrote:
> > 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.
> > 
> > v2->v3:
> >  - renamed QDEV handler @unpluggable to @unplug_is_blocked (patches 2/5
> >    and 3/5)
> > 
> > v1->v2:
> >  - introduced unplug blocker (patches 2/5 and 3/5)
> >  - moved fixes to separate patches (see individual changelogs)
> 
> I have some doubts about how hotunplug is handled, but migration
> looks ok.
> Is there a dependency, or can I just pick savevm things meanwhile?
> 

Well... you can drop the unplug blocker patches (2/5 and 3/5) but the
savevm patch (5/5) modifies a function introduced by the unrealize
patch (4/5):

@@ -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);
 }

and not having an unrealize handler calling unregister_savevm() means
unplug will succeed and leave a dangling pointer in the vm_change_state
list... This causes a systematic crash of the source QEMU when attempting
a migration.

> > ---
> > 
> > Greg Kurz (5):
> >       virtio-9p-coth: fix init function
> >       qdev: add the @unplug_is_blocked 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(-)
> 

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

[Michael S. Tsirkin]

On Tue, Oct 20, 2015 at 08:00:53PM +0200, Greg Kurz wrote:
> On Tue, 20 Oct 2015 15:53:08 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Oct 20, 2015 at 11:16:40AM +0200, Greg Kurz wrote:
> > > 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.
> > > 
> > > v2->v3:
> > >  - renamed QDEV handler @unpluggable to @unplug_is_blocked (patches 2/5
> > >    and 3/5)
> > > 
> > > v1->v2:
> > >  - introduced unplug blocker (patches 2/5 and 3/5)
> > >  - moved fixes to separate patches (see individual changelogs)
> > 
> > I have some doubts about how hotunplug is handled, but migration
> > looks ok.
> > Is there a dependency, or can I just pick savevm things meanwhile?
> > 
> 
> Well... you can drop the unplug blocker patches (2/5 and 3/5) but the
> savevm patch (5/5) modifies a function introduced by the unrealize
> patch (4/5):
> 
> @@ -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);
>  }
> 
> and not having an unrealize handler calling unregister_savevm() means
> unplug will succeed and leave a dangling pointer in the vm_change_state
> list... This causes a systematic crash of the source QEMU when attempting
> a migration.

OK ... could you pls re-arrange it so that savevm can be applied first?

> > > ---
> > > 
> > > Greg Kurz (5):
> > >       virtio-9p-coth: fix init function
> > >       qdev: add the @unplug_is_blocked 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(-)
> >