Re: [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method

[John Snow]

On 12/9/19 12:17 PM, Vladimir Sementsov-Ogievskiy wrote:
> 09.12.2019 18:26, John Snow wrote:
>> (off list)
>>

Well, it isn't now, but oh well. :)

>> On 12/9/19 10:22 AM, John Snow wrote:
>>>
>>>
>>> On 12/6/19 5:31 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> 14.05.2019 23:19, John Snow wrote:
>>>>> Shift from looking at every root BDS to *every* BDS. This will migrate
>>>>> bitmaps that are attached to blockdev created nodes instead of just ones
>>>>> attached to emulated storage devices.
>>>>>
>>>>> Note that this will not migrate anonymous or internal-use bitmaps, as
>>>>> those are defined as having no name.
>>>>>
>>>>> This will also fix the Coverity issues Peter Maydell has been asking
>>>>> about for the past several releases, as well as fixing a real bug.
>>>>>
>>>>> Reported-by: Peter Maydell <address@hidden>
>>>>> Reported-by: Coverity 😅
>>>>
>>>> What was the coverity number (I don't believe that it was smile:)?
>>>>
>>>
>>>      Reported-by: Peter Maydell <address@hidden>
>>>      Reported-by: Coverity 😅
>>>      Reported-by: aihua liang <address@hidden>
>>>      Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>      Signed-off-by: John Snow <address@hidden>
>>>      Message-id: address@hidden
>>>      Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652490
>>>      Fixes: Coverity CID 1390625
> 
> Ah, missed it, sorry.
> 

No problem -- it got added for the PR instead of the patch.

>>>      CC: Stefan Hajnoczi <address@hidden>
>>>      Signed-off-by: John Snow <address@hidden>
>>>
>>>
>>>> Do someone know, that this patch fixes very-very-very terrible bug?
>>>>
>>>> Before this patch, here were bdrv_next-based loop, with exists from it,
>>>> but not using bdrv_next_cleanup(). This leads to leaked (incremented) 
>>>> refcnt of
>>>> bds on any failure during this loop!
>>>>
>>>> Now we faced this bug, in Rhel-based Qemu, so I strongly recommend to fix 
>>>> it in Rhel.
>>>
>>> OK, this was fixed for 4.1, and was introduced in b35ebdf076d for
>>> 2.12.0, so all versions between have the problem.
>>>
>>
>> As far as I know, we don't "support" incremental backup for RHEL based
>> packages, because we only support what you can do directly through
>> libvirt. And since RHEL libvirt doesn't have incremental backup, ...
>>
>> I can try to fix it anyway, though, if it makes your life easier especially.
> 
> No, actually, it's no real difference, now we fixed it in our branch.
> So, it's up to you.
> 

OK, thanks for the report, then!

--js