On Tue, 20 Oct 2015 15:42:03 +0300
"Michael S. Tsirkin" <m...@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 <gk...@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 <m...@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)
>
