On 03.09.20 12:13, Fabian Grünbichler wrote:
> On August 21, 2020 3:03 pm, Max Reitz wrote:
>> On 18.02.20 11:07, Fabian Grünbichler wrote:
>>
>> [Sorry :/]
> 
> same, I've been meaning to ping/pick this back up but other stuff got in 
> the way. so thanks for the reminder to get this upstream ;)
> 
>>
>>> picking up on John's in-progress patch series from last summer, this is
>>> a stab at rebasing and adding test cases for the low-hanging fruits:
>>>
>>> - bitmap mirror mode with always/on-success/never bitmap sync mode
>>> - incremental mirror mode as sugar for bitmap + on-success
>>>
>>> Fabian Grünbichler (4):
>>>   mirror: add check for bitmap-mode without bitmap
>>>   mirror: switch to bdrv_dirty_bitmap_merge_internal
>>>   iotests: add test for bitmap mirror
>>>   mirror: move some checks to QMP
>>>
>>> John Snow (2):
>>>   drive-mirror: add support for sync=bitmap mode=never
>>>   drive-mirror: add support for conditional and always bitmap sync modes
>>
>> Looks reasonable to me.  I would indeed merge patches 2 through 4 into a
>> single one, and perhaps switch patches 5 and 6.
>>
>> Also, we still need an S-o-b from John on patch 2.
>>
>> I have one question: When the mirror job completes successfully (or is
>> cancelled “successfully”), the bitmap is always fully cleared when the
>> job completes, right?  (Unless in “never” mode.)
> 
> I have to take a closer look as well, it's been a while ;)

No problem, I’m... *cough* not exactly in a hurry.

> IIRC the idea 
> was that failed mirrors would allow re-using the bitmap for a next 
> attempt, unless the mode is always. we are not using that feature (yet) 
> though (see below).
> 
>> Not that I think we should change the current implementation of “clear
>> sync_bitmap; merge dirty_bitmap into sync_bitmap;”.  Just a question for
>> understanding.
>>
>>
>> Soo...  What’s the plan?
> 
> I'll rebase, squash as suggested and resend next week!

OK :)

> I am not sure how 
> the S-O-B by John is supposed to enter the mix - should I just include 
> it in the squashed patch (which would be partly authored, but 
> not-yet-signed-off by him otherwise?)?

I’m not too sure on the proceedings, actually.  I think it should be
fine if you put his S-o-b there, as long as your patch is somehow based
on a patch that he sent earlier with his S-o-b underneath.  But I’m not
sure.

> do you pick it up once he's replied with one?

Yes, that’s what would be best.

> FWIW, with been running with this for quite a while downstream with no 
> issues, but we are only using the following part:
> 
> - create bitmap(s)
> - (incrementally) replicate storage volume(s) out of band (using ZFS)
> - incrementally drive mirror as part of a live migration of VM
> - drop bitmap(s)
> 
> so no fancy semi-permanent bitmap that gets re-used here. I've been 
> toying with implementing some sort of generic replication feature akin 
> to zfs send/recv though, but given that we only have built-in persistent 
> bitmaps with qcow2 and the chance of some other tool or the user messing 
> up other image formats is high, the safe usage scenarios are a bit 
> limited.

OK.

> we do use such long-running bitmaps for our new backup driver though, 
> and it works quite well there!

Good! :)

Max