Re: [Qemu-devel] [PATCH v3 03/23] block: Connect BlockBackend to BlockDriverState

[Kevin Wolf <kwolf@redhat.com>]

Am 30.09.2014 um 12:56 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> 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 <armbru@redhat.com>
> >> ---
> >>  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