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 <mre...@redhat.com>
>> ---
>> 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
