From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49414) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYvKP-0000ds-1x for qemu-devel@nongnu.org; Tue, 30 Sep 2014 07:10:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XYvKN-0001b8-Vq for qemu-devel@nongnu.org; Tue, 30 Sep 2014 07:10:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49668) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYvKN-0001aS-OT for qemu-devel@nongnu.org; Tue, 30 Sep 2014 07:10:43 -0400 Date: Tue, 30 Sep 2014 13:10:33 +0200 From: Kevin Wolf Message-ID: <20140930111033.GG3943@noname.str.redhat.com> References: <1410891148-28849-1-git-send-email-armbru@redhat.com> <1410891148-28849-4-git-send-email-armbru@redhat.com> <20140930104036.GD3943@noname.str.redhat.com> <87egut5qch.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87egut5qch.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH v3 03/23] block: Connect BlockBackend to BlockDriverState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: benoit.canet@nodalink.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mreitz@redhat.com Am 30.09.2014 um 12:56 hat Markus Armbruster geschrieben: > Kevin Wolf writes: > > > Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: > >> The pointer from BlockBackend to BlockDriverState is a strong > >> reference, managed with bdrv_ref() / bdrv_unref(), the back-pointer is > >> a weak one. > >> > >> Convenience function blk_new_with_bs() creates a BlockBackend with its > >> BlockDriverState. Callers have to unref both. The commit after next > >> will relieve them of the need to unref the BlockDriverState. > >> > >> Complication: due to the silly way drive_del works, we need a way to > >> hide a BlockBackend, just like bdrv_make_anon(). To emphasize its > >> "special" status, give the function a suitably off-putting name: > >> blk_hide_on_behalf_of_do_drive_del(). Unfortunately, hiding turns the > >> BlockBackend's name into the empty string. Can't avoid that without > >> breaking the blk->bs->device_name equals blk->name invariant. > >> > >> The patch adds a memory leak: drive_del while a device model is > >> connected leaks the BlockBackend. Avoiding the leak here is rather > >> hairy, but it'll become straightforward in a few commits, so I mark it > >> FIXME in the code now, and plug it when it's easy. > >> > >> Signed-off-by: Markus Armbruster > >> --- > >> block.c | 10 ++-- > >> block/block-backend.c | 71 ++++++++++++++++++++++- > >> blockdev.c | 21 ++++--- > >> hw/block/xen_disk.c | 8 +-- > >> include/block/block_int.h | 2 + > >> include/sysemu/block-backend.h | 5 ++ > >> qemu-img.c | 125 +++++++++++++++++++---------------------- > >> qemu-io.c | 4 +- > >> qemu-nbd.c | 4 +- > >> 9 files changed, 156 insertions(+), 94 deletions(-) > >> > >> diff --git a/block.c b/block.c > >> index 934881f..7ccf443 100644 > >> --- a/block.c > >> +++ b/block.c > >> @@ -2032,7 +2032,7 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, > >> * This will modify the BlockDriverState fields, and swap contents > >> * between bs_new and bs_old. Both bs_new and bs_old are modified. > >> * > >> - * bs_new is required to be anonymous. > >> + * bs_new must be nameless and not attached to a BlockBackend. > >> * > >> * This function does not create any image files. > >> */ > >> @@ -2051,8 +2051,9 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old) > >> QTAILQ_REMOVE(&graph_bdrv_states, bs_old, node_list); > >> } > >> > >> - /* bs_new must be anonymous and shouldn't have anything fancy enabled */ > >> + /* bs_new must be nameless and shouldn't have anything fancy enabled */ > >> assert(bs_new->device_name[0] == '\0'); > >> + assert(!bs_new->blk); > >> assert(QLIST_EMPTY(&bs_new->dirty_bitmaps)); > >> assert(bs_new->job == NULL); > >> assert(bs_new->dev == NULL); > >> @@ -2068,8 +2069,9 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old) > >> bdrv_move_feature_fields(bs_old, bs_new); > >> bdrv_move_feature_fields(bs_new, &tmp); > >> > >> - /* bs_new shouldn't be in bdrv_states even after the swap! */ > >> + /* bs_new must remain nameless and unattached */ > >> assert(bs_new->device_name[0] == '\0'); > >> + assert(!bs_new->blk); > > > > Taking back my R-b: You tricked us, this assertion doesn't hold true. > > Easy to reproduce by taking a live snapshot. qemu-iotests case 052 > > catches it. Didn't you run it? > > I run "make check-qtest check-block" on every commit before I submit. > No idea what went wrong with this one. When run for raw, it's only 052 that catches it. For qcow2, I got some more failures: 039 040 041 051 052 085 I see the problem: Only 039 and 052 are marked as 'quick', i.e. the rest is already excluded from 'make check-block'. 039 and 052 don't work with cache=none and 'make check-block' uses -nocache, so those are skipped as well. I'll send a patch to remove the -nocache option and let it run with the default options. Kevin