[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v6 03/15] block: Release dirty bitmaps in bdrv_c
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v6 03/15] block: Release dirty bitmaps in bdrv_close() |
Date: |
Mon, 9 Nov 2015 17:21:19 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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
[Qemu-block] [PATCH v6 05/15] iotests: Make redirecting qemu's stderr optional, Max Reitz, 2015/11/04
[Qemu-block] [PATCH v6 06/15] iotests: Add test for eject under NBD server, Max Reitz, 2015/11/04