On 6/17/19 7:37 AM, Vladimir Sementsov-Ogievskiy wrote:
> 08.06.2019 1:26, John Snow wrote:
>>
>>
>> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here is block-dirty-bitmap-remove transaction action.
>>>
>>> It is used to do transactional movement of the bitmap (which is
>>> possible in conjunction with merge command). Transactional bitmap
>>> movement is needed in scenarios with external snapshot, when we don't
>>> want to leave copy of the bitmap in the base image.
>>>
>>
>> Oh, interesting. I see why you want this now. OK, let's do it.
>>
>
>
> Hi John!
>
> Hmm, could you stage it, or should I fix something? Seems I've answered all
> questions.
> We need this for our nearest release and wanting to avoid x-vz- prefixes in
> the API,
> I'd be very grateful if we merge it soon.
>
>
Hi Vladimir,
Sorry, I lost track of this thread. (In the future, if you have
something pressing like a release and I don't seem to have noticed an
email, try sending me an email directly without the word "patch" in the
subject it to get my attention, or come ping me on IRC.)
For this series:
I think this is pretty confusing, as evidenced by how we both
misunderstood what it did. So "block_dirty_bitmap_remove" now removes a
bitmap from storage but might not actually release it. That's a little
surprising, but I see why we want it.
So what really happens is:
1. Remove a bitmap from storage, but actually don't release it; then
2. hide the bitmap we're holding onto (holding the persistence and name
data aside)
3. On success, we release the bitmap.
During this process, taking the bitmap's name away and marking it as
non-persistent helps keep it from getting rewritten to disk or from
having anything else interact with it.
On Failure:
1. unhide:
- Restore the persistent bit
- Restore the name
So we restore the persistent bit, but we don't actually go back through
the trouble of making sure that there isn't a collision on the name. I
suppose we are guaranteed this won't happen because if a bitmap got
added, it MUST have been added AFTER this action, and if we are
aborting, it MUST have been removed before we get here.
.... phew, I think that this works, but isn't obvious that it does.
However, if we ever use "hide" or "unhide" outside of the remove API,
you might actually run into troubles where we collide on the name when
you "unhide" it, and I don't like that very much.
I feel like a "hidden" bitmap probably should still occupy namespace to
avoid this problem. But then, it isn't truly hidden from API queries and
such, and subsequent commands could interact with it... we could add a
new permissions flag, but that starts to get kind of messy.
Is this a problem of naming? do we need to "hide" bitmaps? Could we get
away with something simpler?
We could rename the "migration" bool to be something generic and then
use that.
This way, it keeps its persistence flag and name, but it won't get
flushed back out to disk or anything at the conclusion of the
transaction. This avoids the need for worrying about name collision on
"unhide", and doesn't need any new fields.
During this time, we can also mark the bitmap as BUSY which will prevent
it from being used for any operations. The ones that we could use during
this critical window are:
- BACKUP
- ADD
- CLEAR
- ENABLE
- DISABLE
- MERGE
- REMOVE
Backup, clear, enable, disable, and remove will all fail a BUSY check.
Merge will fail for either the source or the destination being BUSY.
So I think that it's possible to avoid the need for a hide() API right
now, just by using the migration bool (renamed) and the busy status.
I want to be very aware of your time constraints though, so I prepared a
mock-up on top of your patches;
https://github.com/jnsnow/qemu/commit/9b3434cc86bbd1cbd86f9fc845435d8d6883c205
If this seems agreeable to you I'll re-send and stage right away
tomorrow. If you really DO want hide() semantics we can work on those
instead.
--js
