Re: [Qemu-block] block: Dirty bitmaps and COR in bdrv_move_feature_fields()

[John Snow]

On 03/04/2016 04:30 AM, Kevin Wolf wrote:
> Am 03.03.2016 um 21:12 hat John Snow geschrieben:
>> On 03/03/2016 02:08 PM, Markus Armbruster wrote:
>>> John Snow <address@hidden> writes:
>>>
>>>> On 03/02/2016 04:31 AM, Kevin Wolf wrote:
>>>>> Am 01.03.2016 um 17:51 hat John Snow geschrieben:
>>>>>>
>>>>>>
>>>>>> On 03/01/2016 04:15 AM, Kevin Wolf wrote:
>>>>>>> Am 01.03.2016 um 00:23 hat John Snow geschrieben:
>>>>>>>> Rule of thumb: follow the node such that the (node, name) pair given at
>>>>>>>> creation time continues to work to identify the bitmap.
>>>
>>> Yet another ignorant / forgetful question: what is a (node, name) pair?
>>> Since it's "given at creation time", I guess it must be something in
>>> QMP.
>>
>> The node=[node-name|device] and name=[bitmap-name] parameters given at
>> QMP creation. You need both parameters to address the bitmap in the
>> future, so I consider them an address tuple of sorts.
> 
> (node-name, bitmap-name) is the canonical address. device-name is just
> an alias for node-name and it can change over time.
> 

Disagree: whatever the use provided is the canonical address. There may
be no node-name at all. It cannot be the canonical address, currently.

> This is the semantics of node parameters in all other QMP commands on
> the BDS level that take both node-names and device-names, so if we're
> talking about another BDS level command, it should do the same.
> 

Agree here.

>>> I can't help but feel like letting device names name their roots was a
>>> mistake.
>>>
>>
>> Had I realized it would lead to this, I would not have gone forward with
>> it. I followed the path of least resistance for how to identify the
>> drive structural objects which were very much in flux at the time.
>>
>> And I was very new. So was BlockBackend.
>>
>> Problems Problems.
>>
>> (Hilariously, I think that using the "either drive OR node-name but not
>> both" approach that everyone hated would be precisely the most
>> semantically accurate right now.)
> 
> The fundamental part isn't in the QMP syntax, but it is that we created
> a QMP command that can't decide whether it's BDS or BB level.
> 

Yes, which does manifest itself as QMP confusion.

> The command takes arbitrary node-names, which means for me that the
> command is on the BDS level. If you look into the code, that's also
> where it's implemented. It does, however, move towards the top as if it
> was on the BB level. This doesn't make any sense.
> 

Yep...

> What we need to do is to fix the command so that it's clearly one or the
> other. Currently, it's _almost_ on the BDS level, so I'd be inclined to
> make it fully BDS level rather than making it fully BB (and I freely
> admit that a reason for this is that the series I'm working on is long
> enough without completely reworking bitmaps to make it fully BB, and
> moving it fully to the BDS level is a one-liner).
> 

Problems:

- Bitmap-add works at the BDS level.
- drive-backup works at the device level.

Solutions:
- Add a new BB-bitmap management command to complement drive-backup,
realizing that block-dirty-bitmap-add is inadequate for such.
- Implement incrementals for blockdev-backup to consume bitmaps created
by block-dirty-bitmap-add, realizing that drive-backup is inadequate for
such.

Either way we skew the problem for now, there's a significant chunk that
needs to be done somewhere.

> After that, we can discuss extending the interface to introduce BB level
> bitmaps. They should be either a new command, or we introduce a new QMP
> option like 'attach-to': 'node'/'device'. I would definitely not
> overload only based on the name given; device names as an alias for the
> root node are too common to make this not confusing.
> 
>> All of the existing documentation uses drive names to reference the
>> creation and management of bitmaps, and the only supported interface for
>> consuming the bitmaps is /drive-backup/, which again only takes
>> backends. blockdev-backup does not currently support naming a bitmap or
>> the incremental backup mode.
>>
>> So it would be odd to tilt the only add mechanism in favor of the node,
>> when the only usage mechanism uses devices exclusively.
>>
>> This is why I wonder if we can't attach bitmaps to both BlockBackends
>> *and* BDS nodes,
> 
> We can. Except that someone (TM) needs to implement it.
> 

Hello, is it me you're looking for?

>> based on what the user provides in the block-dirty-bitmap-add command.
> 
> But preferably not by inferring from the given node or device name.
> I think that would be a bad idea.
> 

Will concede this idea.

> Kevin
> 

--js