hw/block/xen-block.c | 9 +++++++++ 1 file changed, 9 insertions(+)
From: Anthony PERARD <anthony.perard@citrix.com>
Whenever a Xen block device is detach via xenstore, the image
associated with it remained open by the backend QEMU and an error is
logged:
qemu-system-i386: failed to destroy drive: Node xvdz-qcow2 is in use
This happened since object_unparent() doesn't immediately frees the
object and thus keep a reference to the node we are trying to free.
The reference is hold by the "drive" property and the call
xen_block_drive_destroy() fails.
In order to fix that, we call drain_call_rcu() to run the callback
setup by bus_remove_child() via object_unparent().
Fixes: 2d24a6466154 ("device-core: use RCU for list of children of a bus")
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
CCing people whom introduced/reviewed the change to use RCU to give
them a chance to say if the change is fine.
---
hw/block/xen-block.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index a3b69e27096f..fe5f828e2d25 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -972,6 +972,15 @@ static void xen_block_device_destroy(XenBackendInstance *backend,
object_unparent(OBJECT(xendev));
+ /*
+ * Drall all pending RCU callbacks as object_unparent() frees `xendev'
+ * in a RCU callback.
+ * And due to the property "drive" still existing in `xendev', we
+ * cann't destroy the XenBlockDrive associated with `xendev' with
+ * xen_block_drive_destroy() below.
+ */
+ drain_call_rcu();
+
if (iothread) {
xen_block_iothread_destroy(iothread, errp);
if (*errp) {
--
Anthony PERARD
On 08/03/21 15:32, Anthony PERARD wrote: > From: Anthony PERARD <anthony.perard@citrix.com> > > Whenever a Xen block device is detach via xenstore, the image > associated with it remained open by the backend QEMU and an error is > logged: > qemu-system-i386: failed to destroy drive: Node xvdz-qcow2 is in use > > This happened since object_unparent() doesn't immediately frees the > object and thus keep a reference to the node we are trying to free. > The reference is hold by the "drive" property and the call > xen_block_drive_destroy() fails. > > In order to fix that, we call drain_call_rcu() to run the callback > setup by bus_remove_child() via object_unparent(). > > Fixes: 2d24a6466154 ("device-core: use RCU for list of children of a bus") > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > CCing people whom introduced/reviewed the change to use RCU to give > them a chance to say if the change is fine. If nothing else works then I guess it's okay, but why can't you do the xen_block_drive_destroy from e.g. an unrealize callback? Paolo > --- > hw/block/xen-block.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c > index a3b69e27096f..fe5f828e2d25 100644 > --- a/hw/block/xen-block.c > +++ b/hw/block/xen-block.c > @@ -972,6 +972,15 @@ static void xen_block_device_destroy(XenBackendInstance *backend, > > object_unparent(OBJECT(xendev)); > > + /* > + * Drall all pending RCU callbacks as object_unparent() frees `xendev' > + * in a RCU callback. > + * And due to the property "drive" still existing in `xendev', we > + * cann't destroy the XenBlockDrive associated with `xendev' with > + * xen_block_drive_destroy() below. > + */ > + drain_call_rcu(); > + > if (iothread) { > xen_block_iothread_destroy(iothread, errp); > if (*errp) { >
On Mon, Mar 08, 2021 at 03:38:49PM +0100, Paolo Bonzini wrote: > On 08/03/21 15:32, Anthony PERARD wrote: > > From: Anthony PERARD <anthony.perard@citrix.com> > > > > Whenever a Xen block device is detach via xenstore, the image > > associated with it remained open by the backend QEMU and an error is > > logged: > > qemu-system-i386: failed to destroy drive: Node xvdz-qcow2 is in use > > > > This happened since object_unparent() doesn't immediately frees the > > object and thus keep a reference to the node we are trying to free. > > The reference is hold by the "drive" property and the call > > xen_block_drive_destroy() fails. > > > > In order to fix that, we call drain_call_rcu() to run the callback > > setup by bus_remove_child() via object_unparent(). > > > > Fixes: 2d24a6466154 ("device-core: use RCU for list of children of a bus") > > > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > --- > > CCing people whom introduced/reviewed the change to use RCU to give > > them a chance to say if the change is fine. > > If nothing else works then I guess it's okay, but why can't you do the > xen_block_drive_destroy from e.g. an unrealize callback? I'm not sure if that's possible. xen_block_device_create/xen_block_device_destroy() is supposed to be equivalent to do those qmp commands: blockdev-add node-name=xvdz-qcow2 driver=qcow2 file={"driver":"file","filename":"disk.qcow2","locking":"off"} device_add id=xvdz driver=xen-disk vdev=xvdz drive=xvdz-qcow2 But I tried to add a call xen_block_drive_destroy from xen_block_unrealize, but that still is called too early, it's called before object_property_del_all() which would delete "drive" and call release_drive() which would free the node. So, no, I don't think we can use an unrealized callback. I though of trying to delete the "drive" property ahead of calling object_unparent() but I didn't figure out how to do so and it's maybe not possible. So either drain_call_rcu or adding call_rcu(xen_block_drive_destroy) seems to be the way, but since xen_block_drive_destroy uses qmp_blockdev_del, it seems better to drain_call_rcu. Cheers, -- Anthony PERARD
On 08/03/21 18:29, Anthony PERARD wrote: >> If nothing else works then I guess it's okay, but why can't you do the >> xen_block_drive_destroy from e.g. an unrealize callback? > > I'm not sure if that's possible. > > xen_block_device_create/xen_block_device_destroy() is supposed to be > equivalent to do those qmp commands: > blockdev-add node-name=xvdz-qcow2 driver=qcow2 file={"driver":"file","filename":"disk.qcow2","locking":"off"} > device_add id=xvdz driver=xen-disk vdev=xvdz drive=xvdz-qcow2 > > But I tried to add a call xen_block_drive_destroy from > xen_block_unrealize, but that still is called too early, it's called > before object_property_del_all() which would delete "drive" and call > release_drive() which would free the node. Can you use blockdev_mark_auto_del? Then you don't have to call xen_block_drive_destroy at all. Paolo > So, no, I don't think we can use an unrealized callback. > > I though of trying to delete the "drive" property ahead of calling > object_unparent() but I didn't figure out how to do so and it's maybe > not possible. > > So either drain_call_rcu or adding call_rcu(xen_block_drive_destroy) > seems to be the way, but since xen_block_drive_destroy uses > qmp_blockdev_del, it seems better to drain_call_rcu. > > Cheers, >
On Mon, Mar 08, 2021 at 06:37:38PM +0100, Paolo Bonzini wrote: > On 08/03/21 18:29, Anthony PERARD wrote: > > > If nothing else works then I guess it's okay, but why can't you do the > > > xen_block_drive_destroy from e.g. an unrealize callback? > > > > I'm not sure if that's possible. > > > > xen_block_device_create/xen_block_device_destroy() is supposed to be > > equivalent to do those qmp commands: > > blockdev-add node-name=xvdz-qcow2 driver=qcow2 file={"driver":"file","filename":"disk.qcow2","locking":"off"} > > device_add id=xvdz driver=xen-disk vdev=xvdz drive=xvdz-qcow2 > > > > But I tried to add a call xen_block_drive_destroy from > > xen_block_unrealize, but that still is called too early, it's called > > before object_property_del_all() which would delete "drive" and call > > release_drive() which would free the node. > > Can you use blockdev_mark_auto_del? Then you don't have to call > xen_block_drive_destroy at all. There is no legacy_dinfo, so blockdev_mark_auto_del doesn't work. -- Anthony PERARD
On 08/03/21 19:14, Anthony PERARD wrote: > On Mon, Mar 08, 2021 at 06:37:38PM +0100, Paolo Bonzini wrote: >> On 08/03/21 18:29, Anthony PERARD wrote: >>>> If nothing else works then I guess it's okay, but why can't you do the >>>> xen_block_drive_destroy from e.g. an unrealize callback? >>> >>> I'm not sure if that's possible. >>> >>> xen_block_device_create/xen_block_device_destroy() is supposed to be >>> equivalent to do those qmp commands: >>> blockdev-add node-name=xvdz-qcow2 driver=qcow2 file={"driver":"file","filename":"disk.qcow2","locking":"off"} >>> device_add id=xvdz driver=xen-disk vdev=xvdz drive=xvdz-qcow2 >>> >>> But I tried to add a call xen_block_drive_destroy from >>> xen_block_unrealize, but that still is called too early, it's called >>> before object_property_del_all() which would delete "drive" and call >>> release_drive() which would free the node. >> >> Can you use blockdev_mark_auto_del? Then you don't have to call >> xen_block_drive_destroy at all. > > There is no legacy_dinfo, so blockdev_mark_auto_del doesn't work. Then I guess it's okay. Perhaps you can rename the function to xen_block_blockdev_destroy so that it's clear it's a blockdev and no drive. Thanks, Paolo
Hi Paul, Stefano, Could one of you could give a Ack to this patch? Thanks, On Mon, Mar 08, 2021 at 02:32:32PM +0000, Anthony PERARD wrote: > From: Anthony PERARD <anthony.perard@citrix.com> > > Whenever a Xen block device is detach via xenstore, the image > associated with it remained open by the backend QEMU and an error is > logged: > qemu-system-i386: failed to destroy drive: Node xvdz-qcow2 is in use > > This happened since object_unparent() doesn't immediately frees the > object and thus keep a reference to the node we are trying to free. > The reference is hold by the "drive" property and the call > xen_block_drive_destroy() fails. > > In order to fix that, we call drain_call_rcu() to run the callback > setup by bus_remove_child() via object_unparent(). > > Fixes: 2d24a6466154 ("device-core: use RCU for list of children of a bus") > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > CCing people whom introduced/reviewed the change to use RCU to give > them a chance to say if the change is fine. > --- > hw/block/xen-block.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c > index a3b69e27096f..fe5f828e2d25 100644 > --- a/hw/block/xen-block.c > +++ b/hw/block/xen-block.c > @@ -972,6 +972,15 @@ static void xen_block_device_destroy(XenBackendInstance *backend, > > object_unparent(OBJECT(xendev)); > > + /* > + * Drall all pending RCU callbacks as object_unparent() frees `xendev' > + * in a RCU callback. > + * And due to the property "drive" still existing in `xendev', we > + * cann't destroy the XenBlockDrive associated with `xendev' with > + * xen_block_drive_destroy() below. > + */ > + drain_call_rcu(); > + > if (iothread) { > xen_block_iothread_destroy(iothread, errp); > if (*errp) { > -- > Anthony PERARD > -- Anthony PERARD
On 08/03/2021 14:32, Anthony PERARD wrote: > From: Anthony PERARD <anthony.perard@citrix.com> > > Whenever a Xen block device is detach via xenstore, the image > associated with it remained open by the backend QEMU and an error is > logged: > qemu-system-i386: failed to destroy drive: Node xvdz-qcow2 is in use > > This happened since object_unparent() doesn't immediately frees the > object and thus keep a reference to the node we are trying to free. > The reference is hold by the "drive" property and the call > xen_block_drive_destroy() fails. > > In order to fix that, we call drain_call_rcu() to run the callback > setup by bus_remove_child() via object_unparent(). > > Fixes: 2d24a6466154 ("device-core: use RCU for list of children of a bus") > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > CCing people whom introduced/reviewed the change to use RCU to give > them a chance to say if the change is fine. > --- > hw/block/xen-block.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c > index a3b69e27096f..fe5f828e2d25 100644 > --- a/hw/block/xen-block.c > +++ b/hw/block/xen-block.c > @@ -972,6 +972,15 @@ static void xen_block_device_destroy(XenBackendInstance *backend, > > object_unparent(OBJECT(xendev)); > > + /* > + * Drall all pending RCU callbacks as object_unparent() frees `xendev' s/Drall/Drain ? > + * in a RCU callback. > + * And due to the property "drive" still existing in `xendev', we > + * cann't destroy the XenBlockDrive associated with `xendev' with s/cann't/can't With those fixed... Reviewed-by: Paul Durrant <paul@xen.org> > + * xen_block_drive_destroy() below. > + */ > + drain_call_rcu(); > + > if (iothread) { > xen_block_iothread_destroy(iothread, errp); > if (*errp) { >
© 2016 - 2024 Red Hat, Inc.