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

[Kevin Wolf]

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.

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

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

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

Kevin