Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal

[Ryan Harper <ryanh@us.ibm.com>]

* Michael S. Tsirkin <mst@redhat.com> [2010-11-08 10:57]:
> On Mon, Nov 08, 2010 at 08:02:50AM -0600, Ryan Harper wrote:
> > * Markus Armbruster <armbru@redhat.com> [2010-11-08 06:04]:
> > > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > > 
> > > > On Mon, Nov 08, 2010 at 11:32:01AM +0100, Markus Armbruster wrote:
> > > >> Ryan Harper <ryanh@us.ibm.com> writes:
> > > >> 
> > > >> > * Markus Armbruster <armbru@redhat.com> [2010-11-06 04:19]:
> > > >> >> Ryan Harper <ryanh@us.ibm.com> writes:
> > > >> >> 
> > > >> >> > * Markus Armbruster <armbru@redhat.com> [2010-11-05 11:11]:
> > > >> >> >> Ryan Harper <ryanh@us.ibm.com> writes:
> > > >> >> >> 
> > > >> >> >> > * Markus Armbruster <armbru@redhat.com> [2010-11-05 08:28]:
> > > >> >> >> >> I'd be fine with any of these:
> > > >> >> >> >> 
> > > >> >> >> >> 1. A new command "device_disconnet ID" (or similar name) to disconnect
> > > >> >> >> >>    device ID from any host parts.  Nice touch: you don't have to know
> > > >> >> >> >>    about the device's host part(s) to disconnect it.  But it might be
> > > >> >> >> >>    more work than the other two.
> > > >> >> >> >
> > > >> >> >> > This is sort of what netdev_del() and drive_unplug() are today; we're
> > > >> >> >> > just saying sever the connection of this device id.   
> > > >> >> >> 
> > > >> >> >> No, I have netdev_del as (3).
> > > >> >> >> 
> > > >> >> >> All three options are "sort of" the same, just different commands with
> > > >> >> >> a common purpose.
> > > >> >> >> 
> > > >> >> >> > I'd like to rename drive_unplug() to blockdev_del() and call it done.  I
> > > >> >> >> > was looking at libvirt and the right call to netdev_del is already
> > > >> >> >> > in-place; I'd just need to re-spin my block patch to call blockdev_del()
> > > >> >> >> > after invoking device_del() to match what is done for net.
> > > >> >> >> 
> > > >> >> >> Unless I'm missing something, you can't just rename: your unplug does
> > > >> >> >> not delete the host part.
> > > >> >> >> 
> > > >> >> >> >> 2. New commands netdev_disconnect, drive_disconnect (or similar names)
> > > >> >> >> >>    to disconnect a host part from a guest device.  Like (1), except you
> > > >> >> >> >>    have to point to the other end of the connection to cut it.
> > > >> >> >> >
> > > >> >> >> > What's the advantage here? We need an additional piece of info (host
> > > >> >> >> > part) in addition to the device id?
> > > >> >> >> 
> > > >> >> >> That's a disadvantage.
> > > >> >> >> 
> > > >> >> >> Possible advantage: implementation could be slightly easier than (1),
> > > >> >> >> because you don't have to find the host parts.
> > > >> >> >> 
> > > >> >> >> >> 3. A new command "drive_del ID" similar to existing netdev_del.  This is
> > > >> >> >> >>    (2) fused with delete.  Conceptual wart: you can't disconnect and
> > > >> >> >> >>    keep the host part around.  Moreover, delete is slightly dangerous,
> > > >> >> >> >>    because it renders any guest device still using the host part
> > > >> >> >> >>    useless.
> > > >> >> >> >
> > > >> >> >> > Hrm, I thought that's what (1) is.
> > > >> >> >> 
> > > >> >> >> No.
> > > >> >> >> 
> > > >> >> >> With (1), the argument is a *device* ID, and we disconnect *all* host
> > > >> >> >> parts connected to this device (typically just one).
> > > >> >> >> 
> > > >> >> >> With (3), the argument is a netdev/drive ID, and disconnect *this* host
> > > >> >> >> part from the peer device.
> > > >> >> >> 
> > > >> >> >> >                                     Well, either (1) or (3); I'd like to
> > > >> >> >> > rename drive_unplug() to blockdev_del() since they're similar function
> > > >> >> >> > w.r.t removing access to the host resource.  And we can invoke them in
> > > >> >> >> > the same way from libvirt (after doing guest notification, remove
> > > >> >> >> > access).
> > > >> >> >> 
> > > >> >> >> I'd call it drive_del for now, to match drive_add.
> > > >> >> >
> > > >> >> > OK, drive_del() and as you mentioned, drive_unplug will take out the
> > > >> >> > block driver, but doesn't remove the dinfo object; that ends up dying
> > > >> >> > when we call the device destructor.  I think for symmetry we'll want
> > > >> >> > drive_del to remove the dinfo object as well.
> > > >> >> 
> > > >> >> Exactly.
> > > >> >> 
> > > >> >> a. bdrv_detach() to zap the pointer from bdrv to qdev
> > > >> >> b. zap the pointer from qdev to bdrv
> > > >> >> c. drive_uninit() to dispose of the host part
> > > >> >
> > > >> > a-c need to be done to match netdev_del symmetry?  How hard of a req is
> > > >> > this?
> > > >> 
> > > >> Without (c), it's not a delete.  And (c) without (b) leaves a dangling
> > > >> pointer.  (c) without (a) fails an assertion in bdrv_delete().
> > > >> 
> > > >> Aside: (b) should probably be folded into bdrv_detach().
> > > >> 
> > > >> >> Step b could be awkward with (3), because you don't know device details.
> > > >> >> I guess you have to search device properties for a drive property
> > > >> >> pointing to bdrv.  I like (1) because it puts that loop in the one place
> > > >> >> where it belongs: qdev core.  (3) duplicates it in every HOSTDEV_del.
> > > >> >> Except for netdev_del, which is special because of VLANs.
> > > >> >> 
> > > >> >> To avoid step b, you could try to keep the bdrv around in a special
> > > >> >> zombie state.  Still have to free the dinfo, but can't use
> > > >> >> drive_uninit() for that then.
> > > >> >> 
> > > >> >> If you think I'm overcomplicating this, feel free to prove me wrong with
> > > >> >> working code :)
> > > >> >
> > > >> > drive_unplug() works as-is today; so it does feel very combursome at
> > > >> > this point.  Other than the name change and agreement on how mgmt should
> > > >> > invoke the command, it's been a long ride to get here.
> > > >> 
> > > >> Sometimes it takes a tough man to make a tender chicken.
> > > >
> > > >> > I'll take my best shot at trying to clean up the other
> > > >> > pointers and objects; though on one of my attempts when I took out the
> > > >> > dinfo() object that didn't go so well; going to have to audit who uses
> > > >> > dinfo and where and what they check before calling it to have a proper
> > > >> > cleanup that doesn't remove the whole device altogether.
> > > >> 
> > > >> Steps a, b, c are the result of my (admittedly quick) audit.
> > > >> 
> > > >> Here's how the various objects are connected to each other:
> > > >> 
> > > >>                contains
> > > >> drivelist    -----------> DriveInfo
> > > >>                                 |
> > > >>                                 | .bdrv
> > > >>                                 | .id == .bdrv->device_name
> > > >>                                 |
> > > >>                contains         V
> > > >> bdrv_states  -----------> BlockDriverState
> > > >>                              |   ^
> > > >>                        .peer |   |
> > > >>                              |   |                          host part
> > > >> -----------------------------|---|-----------------------------------
> > > >>                              |   |                         guest part
> > > >>                              |   | property "drive"
> > > >>                              v   |
> > > >>                           DeviceState
> > > >> 
> > > >> To disconnect host from guest part, you need to cut both pointers.  To
> > > >> delete the host part, you need to delete both objects, BlockDriverState
> > > >> and DriveInfo.
> > > >
> > > >
> > > > If we remove DriveInfo, how can management later detect that guest part
> > > > was deleted?
> > > 
> > > Directly: check whether the qdev is gone.
> > > 
> > > I don't know how to check that indirectly, via DriveInfo.
> > > 
> > > >              If you want symmetry with netdev, it's possible to keep a
> > > > shell of BlockDriverState/DriveInfo around (solving dangling pointer
> > > > problems).
> > > 
> > > netdev_del deletes the host network part:
> > > 
> > >     (qemu) info network
> > >     Devices not on any VLAN:
> > >       net.0: net=10.0.2.0, restricted=n peer=nic.0
> > >       nic.0: model=virtio-net-pci,macaddr=52:54:00:12:34:56 peer=net.0
> > >     (qemu) netdev_del net.0
> > >     (qemu) info network
> > >     Devices not on any VLAN:
> > >       nic.0: model=virtio-net-pci,macaddr=52:54:00:12:34:56 peer=net.0
> > > 
> > > It leaves around the VLAN object.  Since qdev property points to that,
> > > it doesn't dangle.
> > > 
> > > In my opinion, drive_del should make the drive vanish from "info block",
> > 
> > Yeah; that's the right thing to do here.  Let me respin the patch with
> > the name change and the additional work to fix up the pointers and
> > ensure that we don't see the drive in info block.
> 
> Daniel, I'd like your input here: can you live with
> device diappearing from info block and parsing
> qdev tree info to figure out whether device is really gone?

AFAICT, libvirt doesn't look at or use info block at all.

I'd rather not have to add info block to libvirt; but currently I can't
see how else we can determine if we should call drive_unplug if we do a
device_del() and the guest removes it before we call drive_unplug().  

What happens is that the guest removes the device and when we call
drive_unplug() it fails to find the target device (since it was deleted
by the guest). Then we fail the PCiDelDisk and libvirt keeps the device
config around even though the guest has finished removing it.

The only way I see out of this is to either movethe severing of
host/guest before doing the delete (and notification).  Or adding some
code to parse info block and don't bother calling unplug if the
blockdevice is already deleted.


> 
> > > just like netdev_del makes the netdev vanish from "info network".  And
> > > that means deleting it from bdrv_states.  Whether we delete it
> > > alltogether (which is what I sketched), or turn it into a zombie is a
> > > separate question.  Both work for me.
> > 
> > 
> > -- 
> > Ryan Harper
> > Software Engineer; Linux Technology Center
> > IBM Corp., Austin, Tx
> > ryanh@us.ibm.com

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com