* [PATCH] xen-block: Fix removal of backend instance via xenstore
@ 2021-03-08 14:32 Anthony PERARD via
2021-03-08 14:38 ` Paolo Bonzini
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Anthony PERARD via @ 2021-03-08 14:32 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Stefan Hajnoczi, Maxim Levitsky, Anthony PERARD,
Stefano Stabellini, Paul Durrant, Kevin Wolf, Max Reitz,
xen-devel, qemu-block
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] xen-block: Fix removal of backend instance via xenstore
2021-03-08 14:32 [PATCH] xen-block: Fix removal of backend instance via xenstore Anthony PERARD via
@ 2021-03-08 14:38 ` Paolo Bonzini
2021-03-08 17:29 ` Anthony PERARD via
2021-03-22 14:31 ` Anthony PERARD via
2021-03-22 15:03 ` Paul Durrant
2 siblings, 1 reply; 8+ messages in thread
From: Paolo Bonzini @ 2021-03-08 14:38 UTC (permalink / raw)
To: Anthony PERARD, qemu-devel
Cc: Kevin Wolf, Stefano Stabellini, qemu-block, Paul Durrant,
Max Reitz, Stefan Hajnoczi, xen-devel, Maxim Levitsky
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) {
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xen-block: Fix removal of backend instance via xenstore
2021-03-08 14:38 ` Paolo Bonzini
@ 2021-03-08 17:29 ` Anthony PERARD via
2021-03-08 17:37 ` Paolo Bonzini
0 siblings, 1 reply; 8+ messages in thread
From: Anthony PERARD via @ 2021-03-08 17:29 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-devel, Stefan Hajnoczi, Maxim Levitsky, Stefano Stabellini,
Paul Durrant, Kevin Wolf, Max Reitz, xen-devel, qemu-block
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xen-block: Fix removal of backend instance via xenstore
2021-03-08 17:29 ` Anthony PERARD via
@ 2021-03-08 17:37 ` Paolo Bonzini
2021-03-08 18:14 ` Anthony PERARD via
0 siblings, 1 reply; 8+ messages in thread
From: Paolo Bonzini @ 2021-03-08 17:37 UTC (permalink / raw)
To: Anthony PERARD
Cc: Kevin Wolf, Stefano Stabellini, qemu-block, Paul Durrant,
qemu-devel, Maxim Levitsky, Stefan Hajnoczi, xen-devel,
Max Reitz
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,
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xen-block: Fix removal of backend instance via xenstore
2021-03-08 17:37 ` Paolo Bonzini
@ 2021-03-08 18:14 ` Anthony PERARD via
2021-03-08 18:23 ` Paolo Bonzini
0 siblings, 1 reply; 8+ messages in thread
From: Anthony PERARD via @ 2021-03-08 18:14 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-devel, Stefan Hajnoczi, Maxim Levitsky, Stefano Stabellini,
Paul Durrant, Kevin Wolf, Max Reitz, xen-devel, qemu-block
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xen-block: Fix removal of backend instance via xenstore
2021-03-08 18:14 ` Anthony PERARD via
@ 2021-03-08 18:23 ` Paolo Bonzini
0 siblings, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2021-03-08 18:23 UTC (permalink / raw)
To: Anthony PERARD
Cc: Kevin Wolf, Stefano Stabellini, qemu-block, Paul Durrant,
qemu-devel, Maxim Levitsky, Stefan Hajnoczi, xen-devel,
Max Reitz
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xen-block: Fix removal of backend instance via xenstore
2021-03-08 14:32 [PATCH] xen-block: Fix removal of backend instance via xenstore Anthony PERARD via
2021-03-08 14:38 ` Paolo Bonzini
@ 2021-03-22 14:31 ` Anthony PERARD via
2021-03-22 15:03 ` Paul Durrant
2 siblings, 0 replies; 8+ messages in thread
From: Anthony PERARD via @ 2021-03-22 14:31 UTC (permalink / raw)
To: qemu-devel, Stefano Stabellini, Paul Durrant; +Cc: xen-devel, qemu-block
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] xen-block: Fix removal of backend instance via xenstore
2021-03-08 14:32 [PATCH] xen-block: Fix removal of backend instance via xenstore Anthony PERARD via
2021-03-08 14:38 ` Paolo Bonzini
2021-03-22 14:31 ` Anthony PERARD via
@ 2021-03-22 15:03 ` Paul Durrant
2 siblings, 0 replies; 8+ messages in thread
From: Paul Durrant @ 2021-03-22 15:03 UTC (permalink / raw)
To: Anthony PERARD, qemu-devel
Cc: Kevin Wolf, Stefano Stabellini, qemu-block, Max Reitz,
Stefan Hajnoczi, xen-devel, Paolo Bonzini, Maxim Levitsky
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) {
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-03-22 15:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-08 14:32 [PATCH] xen-block: Fix removal of backend instance via xenstore Anthony PERARD via
2021-03-08 14:38 ` Paolo Bonzini
2021-03-08 17:29 ` Anthony PERARD via
2021-03-08 17:37 ` Paolo Bonzini
2021-03-08 18:14 ` Anthony PERARD via
2021-03-08 18:23 ` Paolo Bonzini
2021-03-22 14:31 ` Anthony PERARD via
2021-03-22 15:03 ` Paul Durrant
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).