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

[Markus Armbruster]

Kevin Wolf <address@hidden> 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 <address@hidden>
>> ---
>>  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.

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

I'll look into it, thanks!