[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
Re: [Qemu-block] [PATCH v6 03/15] block: Release dirty bitmaps in bdrv_close()
Mon, 9 Nov 2015 17:47:53 +0100
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0
On 09.11.2015 17:04, Kevin Wolf wrote:
> Am 04.11.2015 um 19:57 hat Max Reitz geschrieben:
>> 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()
> This doesn't sound right. If there was a dirty bitmap, there must be a
> user associated with it. Now that we simply free the bitmap, that user
> has a dangling pointer.
Well, having an assertion there means that we already assumed that case
to be impossible. Even though it isn't, as you yourself describe:
> An exception would be if we knew that the only "user" of this bitmap is
> the monitor because the monitor doesn't actually maintain its own list
> of bitmaps. However, it's doubtful whether bdrv_close() should remove
> something that the QMP client added explicitly.
So you are proposing that bdrv_close() should fail if there are still
dirty bitmaps attached? I don't like that either.
The bitmaps are attached to the BDS, that much is exposed over QMP, too.
If the BDS is released it's only natural to assume that all its bitmaps
are released, too. If you don't want that, you need to make sure that
the monitor has a reference to the BDS itself so the user can defer the
call to blockdev-del until he's/she's ready.
Maybe we need some QMP command to fetch a reference for the monitor for
that to be more usable, I don't know. It will work with blockdev-add
alone, too, though.
>> and drop the assertion in bdrv_delete().
> Why? It should still hold true.
But it does not for user-added bitmaps.
Actually, on master, you can't break that assertion by adding a bitmap
through QMP, but with the BB and media series, you can. And that's
because as of that series, eject will no longer force-call bdrv_close()
(which bypasses the assertion althogether) but bdrv_unref() instead
(leading to bdrv_delete(), and that gets you the assertion).
I'm not really keen on fixing that in that series, though, since I
consider leaking not much better than a failed assertion, especially
considering I'm trying to actually fix it here anyway.
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
[Qemu-block] [PATCH v6 07/15] block: Move BDS close notifiers into BB, Max Reitz, 2015/11/04