Re: [Qemu-devel] [PATCH v8 01/16] block: Release dirty bitmaps in bdrv_close()

[Max Reitz]

On 28.01.2016 04:01, Fam Zheng wrote:
> On Wed, 01/27 18:59, Max Reitz wrote:
>> bdrv_delete() is not very happy about deleting BlockDriverStates with
>> dirty bitmaps still attached to them. In the past, we got around that
>> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and
>> bdrv_close() simply ignoring that condition. We should fix that by
>> releasing all dirty bitmaps in bdrv_close() and drop the assertion in
>> bdrv_delete().
>>
>> Signed-off-by: Max Reitz <address@hidden>
>> Reviewed-by: John Snow <address@hidden>
>> ---
>>  block.c | 37 +++++++++++++++++++++++++++++--------
>>  1 file changed, 29 insertions(+), 8 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 5709d3d..9a31e20 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -88,6 +88,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const 
>> char *filename,
>>                               const BdrvChildRole *child_role, Error **errp);
>>  
>>  static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
>> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs);
>> +
>>  /* If non-zero, use only whitelisted block drivers */
>>  static int use_bdrv_whitelist;
>>  
>> @@ -2157,6 +2159,8 @@ void bdrv_close(BlockDriverState *bs)
>>  
>>      notifier_list_notify(&bs->close_notifiers, bs);
>>  
>> +    bdrv_release_all_dirty_bitmaps(bs);
>> +
>>      if (bs->blk) {
>>          blk_dev_change_media_cb(bs->blk, false);
>>      }
>> @@ -2366,7 +2370,6 @@ static void bdrv_delete(BlockDriverState *bs)
>>      assert(!bs->job);
>>      assert(bdrv_op_blocker_is_empty(bs));
>>      assert(!bs->refcnt);
>> -    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>>  
>>      bdrv_close(bs);
>>  
>> @@ -3582,21 +3585,39 @@ static void 
>> bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>>      }
>>  }
>>  
>> -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap 
>> *bitmap)
>> +static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
>> +                                                  BdrvDirtyBitmap *bitmap)
>>  {
>>      BdrvDirtyBitmap *bm, *next;
>>      QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
>> -        if (bm == bitmap) {
>> +        if (!bitmap || bm == bitmap) {
>>              assert(!bdrv_dirty_bitmap_frozen(bm));
>> -            QLIST_REMOVE(bitmap, list);
>> -            hbitmap_free(bitmap->bitmap);
>> -            g_free(bitmap->name);
>> -            g_free(bitmap);
>> -            return;
>> +            QLIST_REMOVE(bm, list);
>> +            hbitmap_free(bm->bitmap);
>> +            g_free(bm->name);
>> +            g_free(bm);
>> +
>> +            if (bitmap) {
>> +                return;
>> +            }
>>          }
>>      }
>>  }
>>  
>> +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap 
>> *bitmap)
>> +{
>> +    bdrv_do_release_matching_dirty_bitmap(bs, bitmap);
>> +}
>> +
>> +/**
>> + * Release all dirty bitmaps attached to a BDS (for use in bdrv_close()). 
>> There
>> + * must not be any frozen bitmaps attached.
> 
> Should we assert that?

Well, it is asserted in bdrv_do_release_matching_dirty_bitmap().

>                        And IIUC the intention of this function is to release
> all monitor owned (i.e. user created) dirty bitmaps, which must be named. If
> so, can we assert that too?

Probably we can. I don't really know if it would be worth it, though
(even if it isn't much code). There's not much harm in releasing unnamed
bitmaps here, the main issue would be the question "where did they come
from?".

If I do it, I'll rename the function bdrv_release_named_dirty_bitmaps()
and then assert in bdrv_close() that no bitmaps are attached any more.

Max

> 
> Fam
> 
>> + */
>> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
>> +{
>> +    bdrv_do_release_matching_dirty_bitmap(bs, NULL);
>> +}
>> +
>>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>  {
>>      assert(!bdrv_dirty_bitmap_frozen(bitmap));
>> -- 
>> 2.7.0
>>

signature.asc
Description: OpenPGP digital signature