Re: [Qemu-block] [PATCH v2 11/18] block/backup: upgrade copy_bitmap to BdrvDirtyBitmap

[John Snow]

On 7/8/19 8:02 AM, Max Reitz wrote:
> On 05.07.19 18:52, John Snow wrote:
>>
>>
>> On 7/4/19 1:29 PM, Max Reitz wrote:
> 
> [...]
> 
>>> Which brings me to a question: Why is the copy bitmap assigned to the
>>> target in the first place?  Wouldn’t it make more sense on the source?
>>>
>>
>> *cough*;
>>
>> the idea was that the target is *most likely* to be the temporary node
>> created for the purpose of this backup, even though backup does not
>> explicitly create the node.
>>
>> So ... by creating it on the target, we avoid cluttering up the results
>> in block query; and otherwise it doesn't actually matter what node we
>> created it on, really.
>>
>> I can move it over to the source, but the testing code has to get a
>> little smarter in order to find the "right" anonymous bitmap, which is
>> not impossible; but I thought this would actually be a convenience for
>> people.
> 
> You didn’t really say whether you think it makes more sense on the
> source or on the target.
> 

Yeah, a bitmap that isn't recording writes seems to make about equal
sense on either to me; I chose the destination because it was more
likely to be temporary (in the drive-backup case) and I considered this
a temporary bitmap for use by the job.

Organizationally I felt that made sense. I realize it's also not
strictly true for the blockdev-backup case, but it also doesn't matter
terribly.

Semantically, it's a toss-up. Another coder could conceivably one day
decided to re-enable this bitmap and then it would make more sense on
the source. (That coder would be wrong to do that.)

> This is an internal bitmap, so from my understanding, the user better
> not sees it at all.  It should be easy for them to ignore it, regardless
> of which node it is on.  (I don’t consider “clutter” a strong point,
> because, well, most of our current query interfaces are nothing but
> clutter.)  Sure, that makes it a bit difficult for testing, but testing
> shouldn’t really be the focus.

We don't have an API to truly hide bitmaps. Vladimir wanted to add one
but I felt it complicated too much. I still don't really want one, I
don't think it's worth the time.

At the moment, anonymous bitmaps can be seen but because they have no
name, cannot be addressed. They have an implicit permission protection
that way.

(The complication is that Vladimir wanted to hide *named* bitmaps, which
has implications for the namespace that I didn't want to deal with. We
can hide anonymous bitmaps, but we can't do so automatically because
mirror and backup already use anonymous bitmaps that aren't hidden. I
hope people don't rely on seeing them in the query output, but they might.)

> 
> So for me, it comes down to where it makes more sense.  And I think it
> makes more sense on the source, because it flags source clusters to copy.
> 
> Max
> 

If you insist.