Re: [Qemu-devel] Re: kvm: savevm is broken for me

[Glauber Costa <glommer@redhat.com>]

On Sat, Jul 25, 2009 at 12:01:59AM +0200, Alexander Graf wrote:
>
> On 24.07.2009, at 22:09, Glauber Costa wrote:
>
>> On Fri, Jul 24, 2009 at 09:45:28PM +0200, Alexander Graf wrote:
>>>
>>> On 24.07.2009, at 21:42, Glauber Costa wrote:
>>>
>>>> On Fri, Jul 24, 2009 at 04:27:38PM +0200, Alexander Graf wrote:
>>>>>
>>>>> On 24.07.2009, at 16:20, Luiz Capitulino wrote:
>>>>>
>>>>>> On Fri, 24 Jul 2009 16:18:02 +0200
>>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>>
>>>>>>> On 24.07.2009, at 15:54, Luiz Capitulino wrote:
>>>>>>>
>>>>>>>> On Fri, 24 Jul 2009 00:06:13 +0200
>>>>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>
>>>>>>>>> On 23.07.2009, at 23:58, Luiz Capitulino wrote:
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi there,
>>>>>>>>>>
>>>>>>>>>> If I try to 'savevm' with latest Anthony's tree (HEAD
>>>>>>>>>> 6f725c139ae975646c44ace77bf796318a5783da) QEMU will hang and I
>>>>>>>>>> know
>>>>>>>>>> it's not saving anything because vmstat shows no disk activity
>>>>>>>>>> (which
>>>>>>>>>> is the opposite behavior of when 'savevm' works).
>>>>>>>>>>
>>>>>>>>>> According to 'git bisect' the week's winner is:
>>>>>>>>>>
>>>>>>>>>> """
>>>>>>>>>> commit bd8367761236cd5c435598aeb2f1b8240c09b059
>>>>>>>>>> Author: Alexander Graf <agraf@suse.de>
>>>>>>>>>> Date:   Fri Jul 17 13:51:48 2009 +0200
>>>>>>>>>>
>>>>>>>>>> Fake dirty loggin when it's not there
>>>>>>>>>> """
>>>>>>>>>>
>>>>>>>>>> Indeed, reverting this makes 'savevm' work for me.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yikes. I have no idea why, but just revert the patch :-). 
>>>>>>>>> I don't
>>>>>>>>> really need it anymore since we now have dirty logging in PPC.
>>>>>>>>>
>>>>>>>>> Maybe it's the if (is dirty logging really enabled?) in the
>>>>>>>>> beginning?
>>>>>>>>
>>>>>>>> Yes, it's the if. I have removed it and savevm works.
>>>>>>>>
>>>>>>>> Can you submit the fix please?
>>>>>>>
>>>>>>> Eh - what fix? You found that something doesn't set or 
>>>>>>> evaluate the
>>>>>>> flags right. That still doesn't tell us who behaves incorrectly.
>>>>>>
>>>>>> Ah, okay. I thought you meant the 'if' _was_ the bug.
>>>>>>
>>>>>> Better to revert then?
>>>>>
>>>>> Even better yet to find out who's doing something wrong :-).
>>>>
>>>> It's actually quite simple.
>>>> you continue'd instead of break'ing during the dirty log loop,
>>>> which would put us in an infinite loop
>>>>
>>>> Patch follows:
>>>
>>> But who calls kvm_physical_sync_dirty_bitmap with !(mem->flags &
>>> KVM_MEM_LOG_DIRTY_PAGES)? Sounds rather wrong to me.
>> I'd have to dig, but from code inspection,
>> it seems that ram_save_live does it, in its first interaction.
>>
>> This can probably be improved, but I don't think it is wrong semantics
>> anyway. We can sync dirty bitmaps without having mem->flags &  
>> KVM_MEM_LOG_DIRTY_PAGES,
>> and just got nothing written to qemu dirty bitmap.
>>
>> Anyway, even if we decide to fix ram_save_live to not do it,
>> getting into an endless loop in that case is buggy. So it is really  
>> orthogonal.
>
> Looking at the code again it's probably best to remove the if(). Doing a 
> break isn't exactly right either because it could hide dirty logs in  
> later slots.
It's up to you.

As an alternative, we could do start_addr = mem->phys_addr + mem->len,
or something like it, therefore skipping the whole slot.