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

[Greg Kurz <gkurz@linux.vnet.ibm.com>]

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)
>