xen-block: Fix removal of backend instance via xenstore

Commit Message

Anthony PERARD March 8, 2021, 2:32 p.m. UTC

From: Anthony PERARD <[e-mail]>

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 <[e-mail]>
---
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(+)

Comments

Paolo Bonzini March 8, 2021, 2:38 p.m. UTC | #1

On 08/03/21 15:32, Anthony PERARD wrote:
> From: Anthony PERARD <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> 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) {
>

Anthony PERARD March 8, 2021, 5:29 p.m. UTC | #2

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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> > 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,

Paolo Bonzini March 8, 2021, 5:37 p.m. UTC | #3

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

Anthony PERARD March 8, 2021, 6:14 p.m. UTC | #4

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.

Paolo Bonzini March 8, 2021, 6:23 p.m. UTC | #5

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

Anthony PERARD March 22, 2021, 2:31 p.m. UTC | #6

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 <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> 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
>

Paul Durrant March 22, 2021, 3:03 p.m. UTC | #7

On 08/03/2021 14:32, Anthony PERARD wrote:
> From: Anthony PERARD <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> 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 <[e-mail]>

> +     * xen_block_drive_destroy() below.
> +     */
> +    drain_call_rcu();
> +
>       if (iothread) {
>           xen_block_iothread_destroy(iothread, errp);
>           if (*errp) {
>

Patch

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