| Message ID | 20151020091700.25419.56847.stgit@bahia.huguette.org |
|---|---|
| State | New |
| Headers | show |
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)
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) >
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)
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(+)