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

[Kevin Wolf <kwolf@redhat.com>]

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?

You probably need to swap bs->blk in bdrv_move_feature_fields().

Kevin