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

[Markus Armbruster]

John Snow <address@hidden> writes:

> 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:
>>>>>>>
>>>>>>>
>>>>>>> On 02/29/2016 09:36 AM, Kevin Wolf wrote:
>>>>>>>>
>>>>>>>> * Dirty bitmaps:
>>>>>>>>
>>>>>>>>   We're currently trying, and if I'm not mistaken failing, to move 
>>>>>>>> dirty
>>>>>>>>   bitmaps to the top. The (back then one) bitmap was first added to the
>>>>>>>>   list in Paolo's commit a9fc4408, with the following commit message:
>>>>>>>>
>>>>>>>>     While these should not be in use at the time a transaction is
>>>>>>>>     started, a command in the prepare phase of a transaction might have
>>>>>>>>     added them, so they need to be brought over.
>>>>>>>>
>>>>>>>>   At that point, there was no transactionable command that did this in
>>>>>>>>   the prepare phase. Today we have mirror and backup, but op blockers
>>>>>>>>   should prevent them from being mixed with snapshots in a single
>>>>>>>>   transaction, so I can't see how this change had any effect.
>>>>>>>>
>>>>>>>>   The reason why I think we're failing to move dirty bitmaps to the top
>>>>>>>>   today is that we're moving the head of the list to a different object
>>>>>>>>   without updating the prev link in the first element, so in any case
>>>>>>>>   it's buggy today.
>>>>>>>>
>>>>>>>>   I really would like to keep bitmaps on the BDS where they are, but
>>>>>>>>   unfortunately, we also have user-defined bitmaps by now, and if we
>>>>>>>>   change whether they stick with the top level, that's a change that is
>>>>>>>>   visible on the QMP interface.
>>>>>>>>
>>>>>>>>   On the other hand, the QMP interface clearly describes bitmaps as
>>>>>>>>   belonging to a node rather than a BB (you can use node-name, even 
>>>>>>>> with
>>>>>>>>   no BB attached), so moving them could be considered a bug, even if
>>>>>>>>   it is the existing behaviour.
>>>>>>>>
>>>>>>>
>>>>>>> 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.

All right, it's (node-name, bitmap-name).

Why is bitmap-name insufficient?  It's chosen by the user, so it could
be made unique.

>>>>>> That's an option, though not how most (or probably all) QMP commands
>>>>>> work today. Currently, we have two types of commands:
>>>>>>
>>>>>> * BlockBackend level
>>>>>>   These accept only device names. Specifying a node-name is an error.
>>>>>>
>>>>>> * BlockDriverState level
>>>>>>   The preferred addressing for these is node-names, but they accept
>>>>>>   device names as aliases for the corresponding root node.
>>>>>>
>>>>>> Adding a third category that accepts both as well, but has a semantic
>>>>>> difference between device name and node-name might end up confusing. I'm
>>>>>> not totally against it if we think that it's the best way to represent
>>>>>> things that can be on either level, but maybe we should then deprecate
>>>>>> using device names as an alias for commands on the BDS level.
>>>>>>
>>>>>
>>>>> How about thinking of it like this: There is only one behavior for the
>>>>> bitmap creation command. The (node, name) pair used to create the bitmap
>>>>> will remain a valid address for the bitmap for the duration of its
>>>>> existence.
>>>>>
>>>>> In this way, it doesn't "do something different" for a device vs a node
>>>>> name. It always does the same thing. Of course, internally, we know a
>>>>> slightly more nuanced story, but that's just implementation detail.
>>>>
>>>> The logic makes sense and I'm not saying that there is any inherent
>>>> inconsistence in it. The problem I see is in comparison with other,
>>>> existing commands that use a different logic (which in itself is
>>>> consistent as well, but both don't go well together).
>>>>
>>>
>>> OK, understood.
>>>
>>> (I can't help but feel like this might be the only correct way to do
>>> bitmaps, though.)
>> 
>> 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.)

My memory might be busy falsifying itself to support the idea that I've
always been right, but to the degree it still serves: I don't remember
hating the *concept*, but I do remember hating its realization as "must
give exactly one of two arguments" constraint (ugly, not
introspectable), and doubts about its realization as single, overloaded
argument.

It's not the first time we've painted ourselves into a corner that way.
As long as we habitually cast every new interface in stone at first
release, we're setting us up for painful and expensive failure.  We need
to accept and embrace the fact that complex features take time to
analyze and implement.  Start with an experimental interface, and let
things bake properly.  Bless the interface it when the feature is ready
for use.  We'll get "it's complex" and "it's ready" wrong, too, but
hopefully less often.

>> It's simple enough if it's really just a funny way to name the root.
>> 
>> But semantic creep has set in, and we're now grappling with necessity
>> (real or imagined) to move things when the root changes.  That was never
>> *my* intention.  Or if it was, I was an shortsighted fool ;)
>> 
>>>>>>> For bitmaps created using device names this means floating to the top so
>>>>>>> that e.g. (drive0, bitmap0) continues to work and address what you think
>>>>>>> it does.
>>>>>>>
>>>>>>> For intermediary nodes or nodes explicitly referenced by their node name
>>>>>>> instead of their device name, this means it should stick to the node.
>>>>>>>
>>>>>>> All of the BlockDirtyBitmap commands use block_dirty_bitmap_lookup for
>>>>>>> the resolution, which identifies the BDS with bdrv_lookup_bs(node, node,
>>>>>>> NULL) and then tries to find the bitmap on that singular BDS with
>>>>>>> bdrv_find_dirty_bitmap(bs, name).
>>>>>>>
>>>>>>> For bitmaps created with device names, this means that if the bitmaps
>>>>>>> don't float up, we actually lose addressability to the bitmap.
>>>>>>>
>>>>>>> This may imply a new `bool float` property to be filled in at name
>>>>>>> resolution time.
>>>>>>
>>>>>> There are no floating BDS properties, nor is there, generally speaking,
>>>>>> a single top towards which they could be floating. Or to be precise,
>>>>>> there is one currently, but getting rid of that exactly the goal of my
>>>>>> work, because it is a precondition for allowing multiple BBs on a single
>>>>>> BDS.
>>>>>>
>>>>>
>>>>> Yeah, understood. In the case that a bitmap needs to float upwards, it
>>>>> needs to float towards the BB it was named after. I think it's possible
>>>>> to eliminate that ambiguity. If we follow the "We must be able to access
>>>>> the bitmap using the (node, name) pair" rule there's only one option for
>>>>> which node the bitmap floats to -- the root node under the BB it was
>>>>> created against.
>>>>>
>>>>> I think you are against storing pointers to parents in children nodes,
>>>>> so perhaps we can cache the name of the target the bitmap was created
>>>>> against and use that information to know if the bitmap needs to migrate
>>>>> to a new node or not -- and if so, where.
>>>>
>>>> I'm not really only against pointers to parents (though these bring
>>>> additional problems), but really against anything that allows children
>>>> to know their parents. We want to change the parents without the
>>>> children having to care about it.
>>>>
>>>> If something "floats on top", the right design seems to me to implement
>>>> it in the BlockBackend layer rather than implementing it in the BDS and
>>>> trying to adjust when the graph changes.
>>>>
>>>
>>> Yeah, you're right. If we specify a BB name, we should just attach it to
>>> the BB and be done with it. No tomfoolery.
>> 
>> Re tomfoolery: automatically moving stuff from one BDS to another when
>> the user reconfigures the graph is a tar pit.  You need to decide what
>> the "right" move is, and that may involve guessing the user's intent.
>> There's just too much the user might want to do.
>> 
>>> This does differ from our current approach, which always resolves to a
>>> node. However, I think it does not change the semantics of the interface
>>> at all -- (device, name) pairs continue to work as expected. Where we
>>> attach it internally remains a bit of an implementation detail.
>>>
>>> Block query would need to be updated to print bitmaps attached to BBs,
>>> though, but there should be no client making programmatic use of the
>>> query function just yet. (At least, libvirt doesn't.)
>> 
>> Just to make sure I'm still with you: are you proposing to support
>> bitmaps attaching to both BDSes and BBs?
>> 
>
> Well, you'd never have to move them.
>
> (Yes, that is what I am proposing.)

Got it.

> From the viewpoint of: BlockBackends and BlockDriverState objects
> provide different semantic relations to the drive as a whole. Graph-wide
> relationships belong with the BB, driver-specific or filter-specific
> relationships belong with the BDS.
>
> Arguably, we want and need bitmaps in both scenarios, no? We're fuzzing
> drive-wide semantics currently by attaching to the root node, but if
> that bit of hackery goes away, we'll have to go for the Real McCoy.

Can you give a use case for a bitmap attached to a BDS, and another one
for BB?

>>>>>> What this floating really means is that a function will be implemented
>>>>>> at BlockBackend level instead of BlockDriverState level.
>>>>>>
>>>>>> So what I can offer is forbidding to use device names for now,
>>>>>> restricting dirty bitmaps to the node level. And then you can later add
>>>>>> some BB level bitmap code to allow device names again.
>>>>>>
>>>>>
>>>>> Not entirely comfortable with "later," unless it's later in the same
>>>>> pull request. A little bit of breakage might be ok, but an outright
>>>>> regression might be hard to swallow.
>>>>
>>>> Hm, fair enough. Changing dirty bitmaps to work on both layers is
>>>> probably out of scope for my series. Doesn't really matter though,
>>>> because...
>> 
>> Well, you fortuitously broke bitmaps moving to the top "in time for
>> 2.5".  Nobody noticed.  I think that gives you license to break whatever
>> was broken then differently now, and then some.
>> 
>
> Yeah, there's no clear "what to do on reconfiguration" behavior. It
> hasn't been consistent for any number of releases. (...All two!)
>
> What does work, though, is using device names as targets to attach
> bitmaps to, and I would hate to see that break if I can avoid it.

Is anybody using it in anger, yet?  If yes, can we ask for forgiveness?

I'm not saying you *should* break things.  I'm merely challenging the
"must not break anything" dogma.  Let's be pragmatic.

>>>>>>>>   I can imagine use cases for both ways, so the interface that would
>>>>>>>>   make the most sense to me is to generally keep BDSes at their node,
>>>>>>>>   and to provide a QMP command to move them to a different one.
>>>>>>>>
>>>>>>>>   With compatibility in mind, this seems to be a reall tough one,
>>>>>>>>   though.
>>>>>>>
>>>>>>> I am sure that exceptions, as always, exist. If this breaks any behavior
>>>>>>> I am more than willing to say that this is simply a necessary bugfix and
>>>>>>> document accordingly. We do not even have any libvirt support yet, and
>>>>>>> half of the feature is still being actively concocted right now.
>>>>>>>
>>>>>>> I think we have some right to change behavior for this feature still.
>>>>>>
>>>>>> That's good news.
>>>>>>
>>>>>> Kevin
>>>>>>
>>>>>
>>>>> Post-script:
>>>>>
>>>>> It also occurs to me that as the BB graph becomes more fluid that the
>>>>> "bitmap name is unique per BDS" rule we have may become somewhat
>>>>> inadequate -- what if a bitmap is floated to another node where there is
>>>>> a conflict? should the whole operation fail? what if it is too late to 
>>>>> fail?
>>>>>
>>>>> Maybe we need to enforce /globally/ unique bitmap names for user-created
>>>>> bitmaps to make sure this kind of tomfoolery doesn't occur...
>>>>>
>>>>> ...Well, which might then cause problems with .qcow2 persistence if two
>>>>> different qcow2s use the same bitmap names and are later used within the
>>>>> same QEMU instance.
>>>>>
>>>>> Maybe there needs to be a little bit of namespace mangling going on for
>>>>> that feature.
>>>>
>>>> ...here you're really removing my doubts. You probably didn't intend it
>>>> this way, but what you describe is a very clear sign that letting
>>>> bitmaps float on the top is a bug and we need to get rid of it.
>>>>
>>>> Seriously, if you need back references from BDS to BB, watch graph
>>>> modifications above the node, automatically creating new names for
>>>> bitmaps while taking a snapshot and similar things, then you're doing
>>>> something wrong. I don't even want to start thinking about persistent
>>>> bitmaps floating towards the top.
>>>>
>>>
>>> Fair.
>>>
>>>> So I think what we need to do is simple: Keep the bitmaps at the node
>>>> for which they were created. For the cases where we must change what a
>>>> bitmap tracks, we can provide a QMP command to move it. If any name
>>>> change or anything like that is involved, it needs to be explicitly
>>>> requested by the management tool and not automatically be done by qemu
>>>> magic.
>> 
>> Makes sense to me.
>> 
>>>> Eric, does this look reasonable for libvirt?
>>>>
>>>> Kevin
>>>>
>>>
>>> I'll have to think about that -- The way we've been thinking about it is
>>> to assume that bitmaps created with a device name always "stick to the
>>> BB," which we've fuzzily implemented as always being attached to the
>>> root node under said BB. We have not been paying close attention to what
>>> happens when you provide a node name because we do not have any use
>>> cases for that yet.
>>>
>>> If we introduce the concept that -- if the user is not vigilant -- the
>>> bitmap may accidentally wind up on some intermediary node, we'll have to
>>> re-audit our work so far. I don't think this is the right way to go.
>> 
>> Well, when a user meddles with the graph, he better be vigilant.  I
>> figure the explicit graph-meddling commands will be arcane enough to
>> deter casual use.
>> 
>
> If the user is doing explicit graph-meddling, yes. If it's implicit
> graph meddling as a result from a macro command (A job, for example) I
> think it's wrong to expect fiddling.
>
> So, for example, if we create strange intermediary nodes for the
> purposes of mirroring, backups or so on, I am fervently against
> requiring the user to manage bitmap placement explicitly in reaction to
> implementation detail. The user cannot hope to get this kind of
> configuration correct if they don't even know what's going on in the
> job's black box.

Agreed.

> However, if the user themselves is using low-level QMP primitives to
> fiddle with the graph, great. You have opened Pandora's Turing Machine
> and you are on your own. (Bitmap manipulation primitives here will of
> course be useful.)
>
>> I'm more scared of innocent-looking high-level operations that
>> reconfigure the graph under the hood.  If the user mixes them with his
>> own graph meddling, and doesn't fully understand what they do, vigilance
>> won't save him.
>> 
>>> I think if we want to say something like:
>>>
>>> - If the user provides a BB name, the bitmap attaches to the *BB*,
>>> - If the user provides a node name, the bitmap attaches to the node (and
>>> will always stay on THAT node. QMP commands to move the bitmap in this
>>> scenario are appropriate. As long as the node we attach to always keeps
>>> the name it had when we attached to it, I'm happy.
>> 
>> Now you confused me.  Example: user attaches bitmap to the BDS / node
>> with node name "Alice".  There's a QMP command to move the bitmap to
>> another node.  What does it move?  The bitmap?  The bitmap and the node
>> name?
>> 
>
> User creates bitmap "foobar" on BDS "Alice". The user-visible address
> for accessing this bitmap is now and forever (Alice, foobar).

What if the BDS doesn't have a node-name?

> If the bitmap were ever to migrate to a new node, this means the
> user-visible address has been invalidated, which is a leading cause of
> sadness among digital natives.
>
> What I mean to say with the above is that as long as the bitmap stays
> attached to the node, and the node always keeps that name, I am happy.
> If, for whatever ungodly reason, we incur a graph reconfiguration where
> /names are swapped/, the bitmap needs to follow the name instead of the
> node.

Got it.

> "Can that happen?"
>
> vvvvvv
>
>>>                                                    Any action (if any
>>> exist) that would change the node name need to move the bitmap to
>>> wherever the name went
>> 
>> Do we want to let users change a node's name?
>> 
>
> **I sure as hell hope not**, I just wanted to be explicitly clear about
> what invariant I am trying to maintain here, which is bitmap addressibility.
>
> "What's broken, then?"
>
> Device names -- if the user uses a device name, it goes to the root
> node, but the address is through the BB. Graph reconfiguration may
> actually invalidate this address.
>
> "Let's just get rid of creating bitmaps against device names"
>
> This is what Kevin is proposing, if I am reading him correctly. It's a
> good idea overall, but the doubt I have is over the loss of the semantic
> pairing of a bitmap with a /drive/ versus pairing with a node.

If there is a use case where the bitmap should stay at the top (whatever
"top" may be), the natural way to support it would be attaching it to
the BB (which also disambiguates "top").

If we attach bitmaps to BBs, then interpreting BB names
(a.k.a. "device") as BB names makes sense.

Else, interpreting BB names as sugar for the current root BDS
(a.k.a. "node") might be convenient.  Especially when the node doesn't
have a name.  It might also be confusing, because unlike a node name,
which always names the same BDS, the device name may name different
BDSes over time.

I've grown wary of prematurely added convenience features.  Too often,
they become inconvenience misfeatures as our understanding of the
problem deepens.

> Casual use does not care about which actual node in the storage graph is
> performing the IO, we care about the abstract concept of the drive.
>
> 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.

We're talking about existing interfaces.  We should be talking about use
cases.

> This is why I wonder if we can't attach bitmaps to both BlockBackends
> *and* BDS nodes, based on what the user provides in the
> block-dirty-bitmap-add command.

Forget about the existing interfaces.  Analyze the problem, explore the
solution space, and only then figure out what to do for backwards
compatibility.  Anything else hobbles your mind.

>>>                        -- or just refuse to work on nodes with bitmaps
>>> until the user moves them first. either way.)
>>>
>>> that's sufficient to make me happy.
>>>
>>> The question is the suitability of the current command.
>> 
>> Ignorant me again: what's the current command?
>> 
>
> void qmp_block_dirty_bitmap_add
> (const char *node, const char *name,
> bool has_granularity, uint32_t granularity, Error **errp)
>
> If you use a device name, it resolves to the root node, or you can
> specify an actual node name.

Parameter name "node" strongly suggests that the bitmap is attached to
the (fixed) node, not to the (variable) root node of the device.

Its documentation is unhelpful, though: @node: name of device/node which
the bitmap is tracking.

>>> Is there a concrete argument against making bitmap-add work in this way?
>>> Do we *need* a new command, one for BBs and one for BDSs? We already
>>> overloaded the existing command to work with either -- what's the
>>> benefit to splitting it out now?
>>>
>>> --js
>
> I am hereby proposing as the entirety of my grand vision:
>
> 1) Allow bitmaps to attach to BB or BDS
> 2) Modify block-dirty-bitmap-add to attach to either BB or BDS based on
> the name given without modifying the QMP API

Semantic change.  Always awkward, but since it's been broken for a
release without anybody noticing...

> 3) Remove all bitmap migration code from reconfiguration paths. Do not
> look back.
> 4) Sleep soundly.
>
> I think this differs from Kevin's plan only in my desire to just re-use
> the existing API as dual-purpose for both node-and-device semantics.
>
> I would also really be OK with replacing the "node=" parameter with a
> "device or node-name but not both" flavor if some API breakage is
> desired, but I think having two entirely separate commands might be a
> bit much.

We need to make up our minds: do we want to continue conflating BB and
BDS in the external interface, or do we want to separate them?  This
goes beyond just bitmaps.

> Also feel that bitmap-moving commands are not important until
> blockdev-backup supports per-node incremental backups and we document
> and publish any use cases outside of our full-drives-only story that we
> currently peddle.

We shouldn't add features without an actual use.