Re: [Qemu-devel] safety of migration_bitmap_extend

[Dr. David Alan Gilbert]

* Wen Congyang (address@hidden) wrote:
> On 11/03/2015 09:47 PM, Dr. David Alan Gilbert wrote:
> > * Juan Quintela (address@hidden) wrote:
> >> "Dr. David Alan Gilbert" <address@hidden> wrote:
> >>> Hi,
> >>>   I'm trying to understand why migration_bitmap_extend is correct/safe;
> >>> If I understand correctly, you're arguing that:
> >>>
> >>>   1) the migration_bitmap_mutex around the extend, stops any sync's 
> >>> happening
> >>>      and so no new bits will be set during the extend.
> >>>
> >>>   2) If migration sends a page and clears a bitmap entry, it doesn't
> >>>      matter if we lose the 'clear' because we're copying it as
> >>>      we extend it, because losing the clear just means the page
> >>>      gets resent, and so the data is OK.
> >>>
> >>> However, doesn't (2) mean that migration_dirty_pages might be wrong?
> >>> If a page was sent, the bit cleared, and migration_dirty_pages 
> >>> decremented,
> >>> then if we copy over that bitmap and 'set' that bit again then 
> >>> migration_dirty_pages
> >>> is too small; that means that either migration would finish too early,
> >>> or more likely, migration_dirty_pages would wrap-around -ve and
> >>> never finish.
> >>>
> >>> Is there a reason it's really safe?
> >>
> >> No.  It is reasonably safe.  Various values of reasonably.
> >>
> >> migration_dirty_pages should never arrive at values near zero.  Because
> >> we move to the completion stage way before it gets a value near zero.
> >> (We could have very, very bad luck, as in it is not safe).
> > 
> > That's only true if we hit the qemu_file_rate_limit() in ram_save_iterate;
> > if we don't hit the rate limit (e.g. because we're CPU or network limited
> > to slower than the set limit) then I think ram_save_iterate will go all the
> > way to sending every page; if that happens it'll go once more
> > around the main migration loop, and call the pending routine, and now get
> > a -ve (very +ve) number of pending pages, so continuously do 
> > ram_save_iterate
> > again.
> > 
> > We've had that type of bug before when we messed up the dirty-pages 
> > calculation
> > during hotplug.
> 
> IIUC, migration_bitmap_extend() is called when migration is running, and we 
> hotplug
> a device.
> 
> In this case, I think we hold the iothread mutex when 
> migration_bitmap_extend() is called.
> 
> ram_save_complete() is also protected by the iothread mutex.
>
> So if migration_bitmap_extend() is called, the migration thread may be 
> blocked in
> migration_completion() and wait it. qemu_savevm_state_complete() will be 
> called after
> migration_completion() returns.

But I don't think ram_save_iterate is protected by that lock, and my concern
is that the dirty-pages calculation is wrong during the iteration phase, and 
then
the iteration phase will never exit and never try and get to ram_save_complete.

Dave

> 
> Thanks
> Wen Congyang
> 
> > 
> >> Now, do we really care if migration_dirty_pages is exact?  Not really,
> >> we just use it to calculate if we should start the throotle or not.
> >> That only test that each 1 second, so if we have written a couple of
> >> pages that we are not accounting for, things should be reasonably safe.
> >>
> >> Once told that, I don't know why we didn't catch that problem during
> >> review (yes, I am guilty here).  Not sure how to really fix it,
> >> thought.  I think that the problem is more theoretical than real, but
> > 
> > Dave
> > 
> >> ....
> >>
> >> Thanks, Juan.
> >>
> >>>
> >>> Dave
> >>>
> >>> --
> >>> Dr. David Alan Gilbert / address@hidden / Manchester, UK
> > --
> > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> > 
> > .
> > 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK