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

[John Snow]

On 02/29/2016 09:36 AM, Kevin Wolf wrote:
> Hi all,
> 
> I'm currently trying to get rid of bdrv_move_feature_fields(), so we can
> finally have more than one BB per BDS. Generally the way to do this is
> to move features from BDS and block.c to BB and block-backend.c.
> However, for two of the features I'm not sure about this:
> 
> * Copy on Read:
> 
>   When Jeff introduced bdrv_append() in commit 8802d1fd, the CoR flag
>   was already moved to the new top level when taking a snapshot. Does
>   anyone remember why it works like that? It doesn't seem to make a lot
>   of sense to me.
> 
>   The use case for manually enabled CoR is to avoid reading data twice
>   from a slow remote image, so we want to save it to a local overlay,
>   say an ISO image accessed via HTTP to a local qcow2 overlay. When
>   taking a snapshot, we end up with a backing chain like this:
> 
>       http <- local.qcow2 <- snap_overlay.qcow2
> 
>   There is no point in performing copy on read from local.qcow2 into
>   snap_overlay.qcow2, we just want to keep copying data from the remote
>   source into local.qcow2.
> 
>   Possible caveat: We would be writing to a backing file, but that's
>   similar to what some block jobs do, so if we design our op blockers to
>   cover this case, it should be fine.
> 
>   I'm actually pretty sure that simply removing COR from the list, and
>   therefore changing the behaviour to not move it to the top any more,
>   is the right thing to do and could be considered a bug fix.
> 
> * 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.

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.

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

> Any comments or ideas how to proceed with those two?
> 
> Kevin
> 

I can draft something up if you'd like.
--js