On Mon, Nov 08, 2010 at 08:02:50AM -0600, Ryan Harper wrote: > * Markus Armbruster <arm...@redhat.com> [2010-11-08 06:04]: > > "Michael S. Tsirkin" <m...@redhat.com> writes: > > > > > On Mon, Nov 08, 2010 at 11:32:01AM +0100, Markus Armbruster wrote: > > >> Ryan Harper <ry...@us.ibm.com> writes: > > >> > > >> > * Markus Armbruster <arm...@redhat.com> [2010-11-06 04:19]: > > >> >> Ryan Harper <ry...@us.ibm.com> writes: > > >> >> > > >> >> > * Markus Armbruster <arm...@redhat.com> [2010-11-05 11:11]: > > >> >> >> Ryan Harper <ry...@us.ibm.com> writes: > > >> >> >> > > >> >> >> > * Markus Armbruster <arm...@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 > ry...@us.ibm.com
