From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54314) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XVPxV-0004tq-JZ for qemu-devel@nongnu.org; Sat, 20 Sep 2014 15:04:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XVPxN-0002kx-Qb for qemu-devel@nongnu.org; Sat, 20 Sep 2014 15:04:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41500) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XVPxN-0002kh-Jb for qemu-devel@nongnu.org; Sat, 20 Sep 2014 15:04:29 -0400 Message-ID: <541DCFAE.1080205@redhat.com> Date: Sat, 20 Sep 2014 21:04:14 +0200 From: Max Reitz MIME-Version: 1.0 References: <1410891148-28849-1-git-send-email-armbru@redhat.com> <1410891148-28849-3-git-send-email-armbru@redhat.com> In-Reply-To: <1410891148-28849-3-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 02/23] block: New BlockBackend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, benoit.canet@nodalink.com, stefanha@redhat.com On 16.09.2014 20:12, Markus Armbruster wrote: > A block device consists of a frontend device model and a backend. > > A block backend has a tree of block drivers doing the actual work. > The tree is managed by the block layer. > > We currently use a single abstraction BlockDriverState both for tree > nodes and the backend as a whole. Drawbacks: > > * Its API includes both stuff that makes sense only at the block > backend level (root of the tree) and stuff that's only for use > within the block layer. This makes the API bigger and more complex > than necessary. Moreover, it's not obvious which interfaces are > meant for device models, and which really aren't. > > * Since device models keep a reference to their backend, the backend > object can't just be destroyed. But for media change, we need to > replace the tree. Our solution is to make the BlockDriverState > generic, with actual driver state in a separate object, pointed to > by member opaque. That lets us replace the tree by deinitializing > and reinitializing its root. This special need of the root makes > the data structure awkward everywhere in the tree. > > The general plan is to separate the APIs into "block backend", for use > by device models, monitor and whatever other code dealing with block > backends, and "block driver", for use by the block layer and whatever > other code (if any) dealing with trees and tree nodes. > > Code dealing with block backends, device models in particular, should > become completely oblivious of BlockDriverState. This should let us > clean up both APIs, and the tree data structures. > > This commit is a first step. It creates a minimal "block backend" > API: type BlockBackend and functions to create, destroy and find them. > > BlockBackend objects are created and destroyed exactly when root > BlockDriverState objects are created and destroyed. "Root" in the > sense of "in bdrv_states". They're not yet used for anything; that'll > come shortly. > > BlockBackend is reference-counted. Its reference count never exceeds > one so far, but that's going to change. > > Signed-off-by: Markus Armbruster > --- > block/Makefile.objs | 2 +- > block/block-backend.c | 120 +++++++++++++++++++++++++++++++++++++++++ > blockdev.c | 11 +++- > hw/block/xen_disk.c | 11 ++++ > include/qemu/typedefs.h | 1 + > include/sysemu/block-backend.h | 26 +++++++++ > qemu-img.c | 70 +++++++++++++++++++++--- > qemu-io.c | 8 +++ > qemu-nbd.c | 5 +- > 9 files changed, 243 insertions(+), 11 deletions(-) > create mode 100644 block/block-backend.c > create mode 100644 include/sysemu/block-backend.h [snip] > diff --git a/blockdev.c b/blockdev.c > index c9463e3..583235a 100644 > --- a/blockdev.c > +++ b/blockdev.c [snip] > @@ -1791,6 +1799,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) > } else { > drive_del(dinfo); > } > + blk_unref(blk_by_name(id)); > > aio_context_release(aio_context); > return 0; Well, now if a BB is created by blockdev_init() but not deleted from the monitor, it will not be unref'd. But first of all this is not bad, because the time it should be unref'd is exactly when qemu is exiting anyway and second, the BDS is not unref'd either. Therefore: Reviewed-by: Max Reitz