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

[Max Reitz]

On 09.11.2015 17:21, Max Reitz wrote:
> 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().

OK, I can't actually do that yet. There is a bdrv_close() in one of the
fail paths of bdrv_open_inherit(), and the refcount of that BDS will
probably be 1. There is generally no way that BDS has any bitmaps
attached to it, so we're fine in that regard.

I'll revisit this after this series and its follow-up.

Max

signature.asc
Description: OpenPGP digital signature