Re: [Qemu-devel] [Qemu-block] [PATCH v6 03/15] block: Release dirty bitmaps in bdrv_close()

[Max Reitz]

On 06.11.2015 19:59, John Snow wrote:
> 
> 
> On 11/04/2015 01:57 PM, 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>
>> ---
>>  block.c | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/block.c b/block.c
>> index 3493501..23448ed 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -87,6 +87,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;
>>  
>> @@ -1916,6 +1918,8 @@ void bdrv_close(BlockDriverState *bs)
>>      bdrv_drain(bs); /* in case flush left pending I/O */
>>      notifier_list_notify(&bs->close_notifiers, bs);
>>  
>> +    bdrv_release_all_dirty_bitmaps(bs);
>> +
>>      if (bs->blk) {
>>          blk_dev_change_media_cb(bs->blk, false);
>>      }
>> @@ -2123,7 +2127,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);
>>  
>> @@ -3318,6 +3321,21 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
>> BdrvDirtyBitmap *bitmap)
>>      }
>>  }
>>  
>> +/**
>> + * Release all dirty bitmaps attached to a BDS, independently of whether 
>> they
>> + * are frozen or not (for use in bdrv_close()).
>> + */
> 
> This comment caught my attention ...
> 
>> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
>> +{
>> +    BdrvDirtyBitmap *bm, *next;
>> +    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
>> +        QLIST_REMOVE(bm, list);
>> +        hbitmap_free(bm->bitmap);
>> +        g_free(bm->name);
>> +        g_free(bm);
>> +    }
>> +}
>> +
>>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>  {
>>      assert(!bdrv_dirty_bitmap_frozen(bitmap));
>>
> 
> Currently, the only way a bitmap could/should be frozen is if it is in
> use by a job. If a job is running, you probably shouldn't delete stuff
> it is using out from under it.

Right. Good thing I'm fixing everything and bdrv_close() will no longer
be called unless there are no references to the BDS anymore (and each
block job holds an own reference to its BDS). :-)

(At least that's how it's supposed to be; also, there is an
assert(!bs->job) in bdrv_close())

Also, I wanted to mirror current behavior. Right now, we are just
leaking all dirty bitmaps on bdrv_close() (bdrv_delete() asserts that
there are none, but if you invoke bdrv_close() directly...).

> I am assuming by now that it's actually likely that you've canceled any
> jobs attached to this node before you go through the motions of deleting
> it, and it should be safe to just call bdrv_release_dirty_bitmap ...

Well, yes, but then I'd have to iterate through all the dirty bitmaps to
call bdrv_release_dirty_bitmap() for each of them, and then
bdrv_release_dirty_bitmap() iterates through all of them again to check
whether it really belongs to the BDS. That seemed a bit like a waste.

> We don't want the two foreach loops though, so maybe just factor out a
> helper that bdrv_release_all_dirty_bitmaps and bdrv_release_dirty_bitmap
> can share. You can leave the frozen assertion
> in just bdrv_release_dirty_bitmap before it invokes the helper, so that
> bdrv_delete always succeeds in case something gets out-of-sync in the
> internals.

Hm, yes, that should do just fine, thanks.

> (Hmm, to prevent leaks, if you delete a bitmap that is frozen, you
> should probably call the helper on its child object too... or just make
> the helper recursive.)

I think it'll be actually better to just keep the assertion in and thus
not allow releasing frozen dirty bitmaps in bdrv_close(). In addition
I'll add an assertion that !bs->refcount to bdrv_close().

Max

signature.asc
Description: OpenPGP digital signature