Re: [Qemu-devel] [PATCH 2/9] block-migration: fix pending() return value

[Dr. David Alan Gilbert]

* John Snow (address@hidden) wrote:
> 
> 
> On 12/11/2014 09:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> >Because of wrong return value of .save_live_pending() in
> >block-migration, migration finishes before the whole disk
> >is transferred. Such situation occures when the migration
> (occurs)
> >process is fast enouth, for example when source and dest
> (enough)
> >are on the same host.
> >
> >If in the bulk phase we return something < max_size, we will skip
> >transferring the tail of the device. Currently we have "set pending to
> >BLOCK_SIZE if it is zero" for bulk phase, but there no guarantee, that
> >it will be < max_size.
> >
> >True approach is to return, for example, max_size+1 when we are in the
> >bulk phase.
> >
> >Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> >---
> >  block-migration.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/block-migration.c b/block-migration.c
> >index 6f3aa18..8df30d9 100644
> >--- a/block-migration.c
> >+++ b/block-migration.c
> >@@ -757,8 +757,8 @@ static uint64_t block_save_pending(QEMUFile *f, void 
> >*opaque, uint64_t max_size)
> >                         block_mig_state.read_done * BLOCK_SIZE;
> >
> >      /* Report at least one block pending during bulk phase */
> >-    if (pending == 0 && !block_mig_state.bulk_completed) {
> >-        pending = BLOCK_SIZE;
> >+    if (pending <= max_size && !block_mig_state.bulk_completed) {
> >+        pending = max_size + BLOCK_SIZE;
> >      }
> >      blk_mig_unlock();
> >      qemu_mutex_unlock_iothread();
> >
> 
> This looks sane to me, but I am CC'ing the Migration maintainers to give it
> a proper look.
> 
> Looks to me like this is to prevent this from happening, in
> migration/migration.c:
> 
>             pending_size = qemu_savevm_state_pending(s->file, max_size);
>             trace_migrate_pending(pending_size, max_size);
>             if (pending_size && pending_size >= max_size) {
> 
> If we fail that condition, we omit data because we do not call
> qemu_savevm_state_iterate.

Yes, I think this is safe, it doesn't feel nice, but it's safe, and I don't
know of a nicer way unless that is it could calculate the actual amount of
pending data.
The other thing that would feel safe would be to make the _complete handler
check to see if the bulk stage has completed and if it hasn't complete it.

Dave
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK