Re: [Qemu-devel] [RFC] dirty-bitmaps: add block-dirty-bitmap-persist command

[John Snow]

On 8/13/19 8:08 PM, Eric Blake wrote:
> On 8/13/19 5:44 PM, John Snow wrote:
>> This is for the purpose of toggling on/off persistence on a bitmap.
>> This enables you to save a bitmap that was not persistent, but may
>> have already accumulated valuable data.
>>
>> This is simply a QOL enhancement:
>> - Allows user to "upgrade" an existing bitmap to persistent
>> - Allows user to "downgrade" an existing bitmap to transient,
>>   removing it from storage without deleting the bitmap.
>>
> 
> In the meantime, a workaround is:
> 
> create tmp bitmap (non-persistent is fine)
> merge existing bitmap into tmp bitmap
> delete existing bitmap
> recreate original bitmap with desired change in persistence
> merge tmp bitmap into re-created original bitmap
> delete tmp bitmap
> 

Merge really lets us get away with a lot :) It's a powerful command. And
now that merge supports cross-granularities, you can even use it to
change the granularity of a bitmap.

> (I'm not sure how much, if any of that, has to be done with a
> transaction; ideally none, since merging two bitmaps that are both
> enabled is not going to lose any bits.  And since one of the two ends of
> the transaction has a non-persistent bitmap, qemu failing in the narrow
> window where the original bitmap does not exist at all is not that much
> different from failing while the bitmap is transient. If losing data due
> to qemu failure was important, the bitmap should never have been
> transient in the first place)
> 

Yup, quite a lengthy workaround, but it _IS_ possible, it's just not
very nice.

>> Signed-off-by: John Snow <address@hidden>
>> ---
>>
>> This is just an RFC because I'm not sure if I really want to pursue
>> adding this, but it was raised in a discussion I had recently that it
>> was a little annoying as an API design that persistence couldn't be
>> changed after addition, so I wanted to see how much code it would take
>> to address that.
>>
>> (So this patch isn't really tested; just: "Hey, look!")
>>
>> I don't like this patch because it exacerbates my perceived problems
>> with the "check if I can make it persistent, then toggle the flag"
>> model, where I prefer the "Just try to set it persistent and let it fail
>> if it cannot" model, but there were some issues with that patchset that
>> I want to revisit.
> 
> The idea itself makes sense. I don't know if libvirt would ever use it,
> but it does seem like it could make hand-management of bitmaps easier to
> reason about.
> 

Right, this isn't for libvirt as much as it is people doing manual
configuration with e.g. qmp-shell (or heaven forbid, their own QMP tooling.)

>> +++ b/qapi/block-core.json
>> @@ -2001,6 +2001,19 @@
>>    'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
>>              '*persistent': 'bool', '*autoload': 'bool', '*disabled': 'bool' 
>> } }
>>  
>> +##
>> +# @BlockDirtyBitmapPersist:
> 
> The QAPI additions look fine to me, regardless of whether you respin the
> code based on review there.
> 

Thanks, just wanted this one out there on the record.

--js