[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from devic
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal |
Date: |
Mon, 8 Nov 2010 18:34:13 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Mon, Nov 08, 2010 at 01:03:18PM +0100, Markus Armbruster wrote:
> "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.
With info qdev?
I am not at all sure we want management to do that, it'll
require that we keep the output stable.
info block is already parsed, it's easier for management
to look there.
> 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",
> just like netdev_del makes the netdev vanish from "info network".
Yes but we need to have something left on info block IMO.
> 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.
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, (continued)
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, Ryan Harper, 2010/11/07
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, Markus Armbruster, 2010/11/08
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, Michael S. Tsirkin, 2010/11/08
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, Markus Armbruster, 2010/11/08
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, Ryan Harper, 2010/11/08
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, Michael S. Tsirkin, 2010/11/08
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, Daniel P. Berrange, 2010/11/08
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, Ryan Harper, 2010/11/08
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, Ryan Harper, 2010/11/08
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, Daniel P. Berrange, 2010/11/08
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, Michael S. Tsirkin, 2010/11/05
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, Ryan Harper, 2010/11/05
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, Markus Armbruster, 2010/11/05
- Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal, Michael S. Tsirkin, 2010/11/08