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

["Michael S. Tsirkin" <mst@redhat.com>]

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?

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