Re: [Qemu-block] [Qemu-devel] qcow2 autoloading bitmaps

[John Snow]

On 01/11/2018 10:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> 11.01.2018 17:43, Eric Blake wrote:
>> On 01/11/2018 08:26 AM, Vladimir Sementsov-Ogievskiy wrote:
>>
>>> # @autoload: the bitmap will be automatically loaded when the image it
>>> is stored
>>> #            in is opened. This flag may only be specified for persistent
>>> #            bitmaps. Default is false for block-dirty-bitmap-add.
>>> (Since: 2.10)
>>>
>>> so, if we consider only enabled bitmaps it is ok to direct map autoload
>>> <=> auto.
>>>
>>> But now we've faced into necessity of load/store disabled bitmaps.
>>>
>>> Current behavior is definitely wrong: user sets autoload flag for
>>> disabled bitmap, but on next
>>> Qemu start the bitmap will be autoloaded and enabled.
>> Can we make it an error to set autoload to true when requesting a
>> disabled bitmap? Or can we not even request a disabled bitmap?
> 
> currently, autoload is set on bitmap creation, so it is always enabled
> at this time.
> 
>>
>>>
>>> Proposed solution:
>>>  - deprecate @autoload flag for bitmap creation, ignore it
>> Is there ever a case where you'd create an enabled persistent
>> dirty-tracking bitmap, but not want it to be re-enabled the next time
>> the image is created?
> 
> looks like we do not. For now we use only persistent+autoload.
> 
>> Your argument for ignoring/deleting the parameter
>> is that you can decide whether to set the "auto" flag solely based on
>> whether the bitmap is both persistent and enabled, without needing any
>> additional user input?
> 
> The main point is that, when we want to load/store disabled bitmaps this
> parameter is confusing: In user's point of view it's ok to have autoloading
> disabled bitmap, which he wants to be autoloaded and disabled on next Qemu
> start. So, to maintain this flag (autoload) for disabled bitmaps, we
> will need
> to add some additional flag "disabled" to Qcow2 spec, to make it possible to
> have "auto" but "disabled" bitmaps. And we will have to change qcow2 "auto"
> specification, to consider "disabled" bit set in parallel. But all this
> looks
> superfluous for now. We already have type "dirty tracking bitmaps" in qcow2
> spec for dirty bitmaps. So, for now, "auto" flag set on "dirty tracking
> bitmaps" means that it is enabled. It is logical to assume that all other
> "dirty tracking bitmaps" are disabled.
> 

Oh, so you want to be able to "autoload" a disabled bitmap, is that right?

Is it important to distinguish these two cases?

(1) A bitmap is saved to the qcow2, but autoload is set to false
(2) A bitmap is saved to the qcow2, autoload is true, but a (new)
disabled bit is set.

Is it okay, basically, to always autoload bitmaps in a disabled state?
Is it imperative we add the new bit?

(Genuinely asking. I'm not sure.)

> On the other hand, "autoloading" is a bad property for disabled bitmap
> at all.
> Disabled bitmap is simpler than enabled (or "auto") bitmap, it doesn't
> require
> any care like "auto" bitmap. So, it is more natural to let Qemu decide
> to load
> or not any disabled bitmaps.
> 

If we don't autoload the disabled ones, I imagine you'll eventually want
some way to manually load them into memory to manipulate them for
various reasons.

> 
> 
>>
>>>  - save persistent enabled bitmaps with "auto" flag
>>>  - save persistent disabled bitmaps without "auto" flag
>>>  - on Qemu start load all bitmaps, mapping "auto" flag state to
>>> "enabled" state.
>> So if I follow the argument, the state of the 'auto' flag stored into
>> the file on BDS close (often at qemu exit) is based on whether the
>> bitmap was currently enabled, and the user can then control whether to
>> enable/disable the bitmap on the fly to control whether the 'auto' flag
>> is stored; thus, having the flag at creation time is redundant.
> 
> Yes.
> 
>>
>>>
>>> Note: we may store a lot of disabled bitmaps in qcow2 image, but loading
>>> them all into RAM may be
>>> inefficient. Actually such bitmap will be needed only on demand (for
>>> export through nbd or
>>> making some kind of backup). So in future it may be optimized by "lazy
>>> load" of disabled bitmaps,
>>> postponing their actual load up to first read or enabling. This
>>> optimization doesn't need changing
>>> of qapi or qcow2 format (at first sight).
>>>
>>>
>>> Note2: now there is no way to disable/enable bitmaps, but there is a
>>>   [PATCH for-2.12 0/4] qmp dirty bitmap API
>>> with big conversation, but I hope, I'll post a new version with a small
>>> fix soon and it will be merged.
>> In other words, you can incorporate your QAPI tweaks proposed here into
>> your respin of that series.
>>
> 
> right idea, I'll do so, just wait a bit for others to comment here.
> 

Stay tuned...

> -- 
> Best regards,
> Vladimir
>