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

[Ryan Harper]

* Markus Armbruster <address@hidden> [2010-11-08 06:04]:
> "Michael S. Tsirkin" <address@hidden> writes:
> 
> > On Mon, Nov 08, 2010 at 11:32:01AM +0100, Markus Armbruster wrote:
> >> Ryan Harper <address@hidden> writes:
> >> 
> >> > * Markus Armbruster <address@hidden> [2010-11-06 04:19]:
> >> >> Ryan Harper <address@hidden> writes:
> >> >> 
> >> >> > * Markus Armbruster <address@hidden> [2010-11-05 11:11]:
> >> >> >> Ryan Harper <address@hidden> writes:
> >> >> >> 
> >> >> >> > * Markus Armbruster <address@hidden> [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.

> 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
address@hidden