From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=53428 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PFPI5-0001hG-HB for qemu-devel@nongnu.org; Mon, 08 Nov 2010 05:49:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PFPI3-0006Wr-Lu for qemu-devel@nongnu.org; Mon, 08 Nov 2010 05:49:33 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36951) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PFPI3-0006WP-Bu for qemu-devel@nongnu.org; Mon, 08 Nov 2010 05:49:31 -0500 Date: Mon, 8 Nov 2010 12:49:06 +0200 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal Message-ID: <20101108104906.GA855@redhat.com> References: <20101103205929.GF3469@us.ibm.com> <20101103212640.GB20833@redhat.com> <20101104164551.GB20081@us.ibm.com> <20101105142558.GG22381@us.ibm.com> <20101105162218.GM22381@us.ibm.com> <20101108021926.GV22381@us.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , yamahata@valinux.co.jp, qemu-devel@nongnu.org, Anthony Liguori , Ryan Harper , Stefan Hajnoczi On Mon, Nov 08, 2010 at 11:32:01AM +0100, Markus Armbruster wrote: > Ryan Harper writes: > > > * Markus Armbruster [2010-11-06 04:19]: > >> Ryan Harper writes: > >> > >> > * Markus Armbruster [2010-11-05 11:11]: > >> >> Ryan Harper writes: > >> >> > >> >> > * Markus Armbruster [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? If you want symmetry with netdev, it's possible to keep a shell of BlockDriverState/DriveInfo around (solving dangling pointer problems). -- MST