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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]

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.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]