[Qemu-block] Closing Bitmaps (Was: Re: [Qemu-devel] [PATCH v7 03/24] block: Release dirty bitmaps in bdrv_close())

[John Snow]

On 11/16/2015 11:22 PM, Fam Zheng wrote:
> On Mon, 11/16 12:07, John Snow wrote:
>>
>>
>> On 11/15/2015 08:27 PM, Fam Zheng wrote:
>>> On Fri, 11/13 17:49, John Snow wrote:
>>>>
>>>>
>>>> On 11/12/2015 01:23 AM, Fam Zheng wrote:
>>>>> On Mon, 11/09 23:39, 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().
>>>>>
>>>>> What bitmaps are attached when bdrv_close() is called?  The ones created 
>>>>> from
>>>>> the monitor should probably be removed by the monitor, and the internal 
>>>>> ones
>>>>> like in migration and block jobs should probably be removed by stopping 
>>>>> the
>>>>> respective job.
>>>>>
>>>>> Fam
>>>>>
>>>>
>>>> Well in this case at least it appears we are still asserting that the
>>>> BDS has no job attached, so it shouldn't have any internal bitmaps
>>>> weighing it down, which just leaves the ones created by the QMP interface.
>>>>
>>>> How important is it that we ask the user to remove all of those bitmaps
>>>> themselves?
>>>>
>>>> It might become more important in the future when persistence is an
>>>> option and we go to close a transient bitmap -- but persistent bitmaps I
>>>> am sure it will be safe to just close out and flush to disk.
>>>>
>>>
>>> Yes, I was not saying we should expecting user to do that manually, but it
>>> would be good to be clear about different types of instances in the code.
>>>
>>> For now, it's unlikely a problem.
>>>
>>> Fam
>>>
>>
>> OK, just pointing out that I think it's unlikely we have any internal
>> bitmaps at this point since we assert that we have no jobs.
>>
>> We can actually test this, since internal bitmaps are anonymous and
>> user-created ones must always have a name.
>>
>> Tangent question: If a user closes a BDS node with a transient bitmap
>> attached, should we take any special action?
>>
>> i.e.; do we offer the user a last chance to save the bitmap somewhere,
>> or do we just do what we were asked and hope the user is competent?
>>
>> (I assume: No, we let the user shoot themselves in the foot if they want
>> to, but I wanted to ask the question.)
>>
>> --js
>>
> 
> For persistent dirty bitmaps, we must provide an interface with which user can
> make sure that, upon the shutting down of VM, the dirty bitmap file is in
> consistent with the image it is tracking.
> 
> With embedded dirty bitmap in qcow2 image, this shouldn't be a problem though,
> because qcow2_close can handle this internally.  But what about the senario
> with "raw image" + "incremental backup"?
> 
> Fam
> 

Still the subject of debate on-list, but the thought is roughly this:

Bitmaps will be able to flush-to-file on close. (If they have no
persistence data, it's a no-op (maybe.)) This might mean being flushed
to their own BDS -- the one they are describing -- as a qcow2 extension.
Or, it could be to an arbitrary new standalone file format designed for
the sole purpose of containing bitmap data.

The discussion hasn't progressed beyond "Max and Kevin do not think
storing arbitrary bitmaps in .qcow2 files is a good idea." The logical
conclusion is "We need a new standalone format, then" but we haven't
decided what that format will look like or how it will be used.

Then, Through CLI arguments or QMP arguments, you can modify the
persistence attributes of bitmaps -- choosing where to store them.
Bitmaps for qcow2 nodes can be stored in their own node, bitmaps
describing raw files will need to be stored in an external file.

(Is it possible to create a block driver that doesn't implement
read/write primitives, and only implements theoretical bitmap load/save
primitives? We could create a block driver for a standalone bitmap
container in this way...)

--js