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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> 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.
>
> 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.

Yes.

>> > 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.
>
> 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.
>
> 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

... and declare the (broken) move to the top a bug ... 

> 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).
>
> 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.

Makes sense to me.

>> 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.

That someone guy is a lazy bum.  He ought to be fired.

>> 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.

Yes.  Feels like too much magic of the wrong kind given the existing
conventions.