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

["Michael S. Tsirkin" <mst@redhat.com>]

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)