: Make migration handle errors a little better

PATCH Applied Chris Lalancette 3 participants 5 messages Jan 23, 2008 – Jan 27, 2008 3 messages not in the archive KVM

Share & tools

Chris Lalancette

author

2008-01-23 19:46 UTC #0 lore raw

PATCH 1/1 : Make migration handle errors a little better

All,
Attached is a fairly simple patch to the migration code to make it handle
errors better. In particular, if the remote side wasn't set up properly (i.e. the memory size didn't match), the migration would fail, but the source host didn't actually react properly, so the migration would still be "in-progress". This patch fixes up the error checking to properly quit when the remote side isn't prepared for whatever reason.

Signed-off-by Chris Lalancette <[email protected]>

qemu/migration.c +19 -6

index 17ad77c..ae95128 100644
@@ -86,6 +86,7 @@ enum { /* migration status values */
 
     MIG_STAT_SAVEVM_FAILED     = 15,
     MIG_STAT_NO_MEM            = 16,
+    MIG_STAT_DIRTY_TRACK_FAIL  = 17,
 
     MIG_STAT_MIGRATION_CANCEL  = 20,

@@ -425,18 +426,26 @@ static int start_migration(MigrationState *s)
 #endif
 	
     r = MIG_STAT_WRITE_FAILED;
-    if (write_whole_buffer(s->fd, &running, sizeof(running)))
+    if (write_whole_buffer(s->fd, &running, sizeof(running))) {
+        perror("vm_running write failed");
         goto out;
-    if (write_whole_buffer(s->fd, &value, sizeof(value)))
+    }
+    if (write_whole_buffer(s->fd, &value, sizeof(value))) {
+        perror("phys_ram_size write failed");
         goto out;
+    }
 
 #ifdef USE_KVM
     if (kvm_allowed) {
         value = cpu_to_be32(n);
-        if (write_whole_buffer(s->fd, &value, sizeof(value)))
+        if (write_whole_buffer(s->fd, &value, sizeof(value))) {
+            perror("phys_ram_size_bitmap size write failed");
             goto out;
-        if (write_whole_buffer(s->fd, phys_ram_page_exist_bitmap, n))
+	}
+        if (write_whole_buffer(s->fd, phys_ram_page_exist_bitmap, n)) {
+            perror("phys_ram_page_exist_bitmap write failed");
             goto out;
+	}
     }
 #endif
     fcntl(s->fd, F_SETFL, O_NONBLOCK);

@@ -456,7 +465,8 @@ static int start_migration(MigrationState *s)
 
     if (cpu_physical_memory_set_dirty_tracking(1)) {
         *s->has_error = MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED;
-        return -1;
+        r = MIG_STAT_DIRTY_TRACK_FAIL;
+        goto out;
     }
 
     s->addr = 0;

@@ -470,6 +480,8 @@ static int start_migration(MigrationState *s)
     qemu_mod_timer(s->timer, qemu_get_clock(rt_clock));
     qemu_set_fd_handler2(s->fd, NULL, NULL, migrate_write, s);
 
+    r = 0;
+
  out:
 #ifdef USE_KVM
     if (phys_ram_page_exist_bitmap)

@@ -498,8 +510,9 @@ static MigrationState *migration_init_fd(int detach, int fd)
 
     current_migration = s;
     
-    if (start_migration(s) == -1) {
+    if (start_migration(s)) {
 	term_printf("Could not start migration\n");
+        migrate_close(s);
 	return NULL;
     }

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

A message that is not in the archive (sent off-list or not yet received): [email protected]

Uri Lublin

2008-01-24 11:01 UTC #2 lore raw

Quoted 10 lines · Chris Lalancette

All,
Attached is a fairly simple patch to the migration code to make it handle
errors better. In particular, if the remote side wasn't set up properly (i.e. the memory size didn't match), the migration would fail, but the source host didn't actually react properly, so the migration would still be "in-progress". This patch fixes up the error checking to properly quit when the remote side isn't prepared for whatever reason.
Signed-off-by Chris Lalancette <[email protected]>

Patch looks good.
Why did you define MIG_STAT_DIRTY_TRACK_FAIL and not MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED ?
Also we need to make sure, in case of a failure, that 'status' is updated and *s->has_error is freed. How about calling migrate_finish ? Thanks,
Uri.

A message that is not in the archive (sent off-list or not yet received): 47987021.6010203-atKUWr5tajBWk0Htik3J/[email protected]

Chris Lalancette

author

2008-01-24 19:22 UTC #4 lore raw

Quoted 10 lines · Uri Lublin

Quoted 8 lines · Chris Lalancette
All,
Attached is a fairly simple patch to the migration code to make it handle
errors better. In particular, if the remote side wasn't set up properly (i.e. the memory size didn't match), the migration would fail, but the source host didn't actually react properly, so the migration would still be "in-progress". This patch fixes up the error checking to properly quit when the remote side isn't prepared for whatever reason.

Quoted 5 lines

Quoted 2 lines
Signed-off-by Chris Lalancette <[email protected]>
Patch looks good.
Why did you define MIG_STAT_DIRTY_TRACK_FAIL and not MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED ?

Heh. I was just being lazy, and I didn't want to type all of that :). I can change it to be more correct, though.

Quoted 2 lines

Also we need to make sure, in case of a failure, that 'status' is updated and *s->has_error is freed. How about calling migrate_finish ?

Oh, good point. I thought calling migrate_close() would be enough, but it looks like you are right, we also need to free up has_error. I'll send an updated patch.

Thanks for the review!
Chris Lalancette

Chris Lalancette

author

2008-01-24 19:41 UTC #5 lore raw

In reply to a message not in the archive

Quoted 3 lines · Uri Lublin

Patch looks good.
Why did you define MIG_STAT_DIRTY_TRACK_FAIL and not MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED ?

Oops, I slightly misunderstood this bit in my last e-mail. You were saying that there is already a "MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED" error flag. In the new patch (attached), I've used this instead.

Quoted 2 lines

Also we need to make sure, in case of a failure, that 'status' is updated and *s->has_error is freed. How about calling migrate_finish ?

And I now call migrate_finish() instead of migrate_close() in the error path, as you suggest. Thanks again!

Signed-off-by Chris Lalancette <[email protected]>

qemu/migration.c +18 -6

index 17ad77c..df0acf9 100644
@@ -425,18 +425,26 @@ static int start_migration(MigrationState *s)
 #endif
 	
     r = MIG_STAT_WRITE_FAILED;
-    if (write_whole_buffer(s->fd, &running, sizeof(running)))
+    if (write_whole_buffer(s->fd, &running, sizeof(running))) {
+        perror("vm_running write failed");
         goto out;
-    if (write_whole_buffer(s->fd, &value, sizeof(value)))
+    }
+    if (write_whole_buffer(s->fd, &value, sizeof(value))) {
+        perror("phys_ram_size write failed");
         goto out;
+    }
 
 #ifdef USE_KVM
     if (kvm_allowed) {
         value = cpu_to_be32(n);
-        if (write_whole_buffer(s->fd, &value, sizeof(value)))
+        if (write_whole_buffer(s->fd, &value, sizeof(value))) {
+            perror("phys_ram_size_bitmap size write failed");
             goto out;
-        if (write_whole_buffer(s->fd, phys_ram_page_exist_bitmap, n))
+	}
+        if (write_whole_buffer(s->fd, phys_ram_page_exist_bitmap, n)) {
+            perror("phys_ram_page_exist_bitmap write failed");
             goto out;
+	}
     }
 #endif
     fcntl(s->fd, F_SETFL, O_NONBLOCK);

@@ -456,7 +464,8 @@ static int start_migration(MigrationState *s)
 
     if (cpu_physical_memory_set_dirty_tracking(1)) {
         *s->has_error = MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED;
-        return -1;
+        r = MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED;
+        goto out;
     }
 
     s->addr = 0;

@@ -470,6 +479,8 @@ static int start_migration(MigrationState *s)
     qemu_mod_timer(s->timer, qemu_get_clock(rt_clock));
     qemu_set_fd_handler2(s->fd, NULL, NULL, migrate_write, s);
 
+    r = 0;
+
  out:
 #ifdef USE_KVM
     if (phys_ram_page_exist_bitmap)

@@ -498,8 +509,9 @@ static MigrationState *migration_init_fd(int detach, int fd)
 
     current_migration = s;
     
-    if (start_migration(s) == -1) {
+    if (start_migration(s)) {
 	term_printf("Could not start migration\n");
+	migrate_finish(s);
 	return NULL;
     }

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

A message that is not in the archive (sent off-list or not yet received): [email protected]

Avi Kivity

Applied

2008-01-27 13:15 UTC #7 lore raw

Quoted 12 lines · Chris Lalancette

Quoted 4 lines · Uri Lublin
Patch looks good.
Why did you define MIG_STAT_DIRTY_TRACK_FAIL and not MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED ?
Oops, I slightly misunderstood this bit in my last e-mail. You were saying that there is already a "MIG_STAT_KVM_SET_DIRTY_TRACKING_FAILED" error flag. In the new patch (attached), I've used this instead.

Applied; thanks.

Signature · 7 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/