| Message ID | 20220216062809.57179-11-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Postcopy Preemption | expand |
* Peter Xu (peterx@redhat.com) wrote: > We used to have quite a few places making sure -EIO happened and that's the > only way to trigger postcopy recovery. That's based on the assumption that > we'll only return -EIO for channel issues. > > It'll work in 99.99% cases but logically that won't cover some corner cases. > One example is e.g. ram_block_from_stream() could fail with an interrupted > network, then -EINVAL will be returned instead of -EIO. > > I remembered Dave Gilbert pointed that out before, but somehow this is > overlooked. Neither did I encounter anything outside the -EIO error. > > However we'd better touch that up before it triggers a rare VM data loss during > live migrating. > > To cover as much those cases as possible, remove the -EIO restriction on > triggering the postcopy recovery, because even if it's not a channel failure, > we can't do anything better than halting QEMU anyway - the corpse of the > process may even be used by a good hand to dig out useful memory regions, or > the admin could simply kill the process later on. > > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > migration/migration.c | 4 ++-- > migration/postcopy-ram.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 6e4cc9cc87..67520d3105 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -2877,7 +2877,7 @@ retry: > out: > res = qemu_file_get_error(rp); > if (res) { > - if (res == -EIO && migration_in_postcopy()) { > + if (res && migration_in_postcopy()) { > /* > * Maybe there is something we can do: it looks like a > * network down issue, and we pause for a recovery. > @@ -3478,7 +3478,7 @@ static MigThrError migration_detect_error(MigrationState *s) > error_free(local_error); > } > > - if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret == -EIO) { > + if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret) { > /* > * For postcopy, we allow the network to be down for a > * while. After that, it can be continued by a > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c > index d3ec22e6de..6be510fea4 100644 > --- a/migration/postcopy-ram.c > +++ b/migration/postcopy-ram.c > @@ -1038,7 +1038,7 @@ retry: > msg.arg.pagefault.address); > if (ret) { > /* May be network failure, try to wait for recovery */ > - if (ret == -EIO && postcopy_pause_fault_thread(mis)) { > + if (postcopy_pause_fault_thread(mis)) { > /* We got reconnected somehow, try to continue */ > goto retry; > } else { > -- > 2.32.0 >
diff --git a/migration/migration.c b/migration/migration.c index 6e4cc9cc87..67520d3105 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2877,7 +2877,7 @@ retry: out: res = qemu_file_get_error(rp); if (res) { - if (res == -EIO && migration_in_postcopy()) { + if (res && migration_in_postcopy()) { /* * Maybe there is something we can do: it looks like a * network down issue, and we pause for a recovery. @@ -3478,7 +3478,7 @@ static MigThrError migration_detect_error(MigrationState *s) error_free(local_error); } - if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret == -EIO) { + if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret) { /* * For postcopy, we allow the network to be down for a * while. After that, it can be continued by a diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index d3ec22e6de..6be510fea4 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -1038,7 +1038,7 @@ retry: msg.arg.pagefault.address); if (ret) { /* May be network failure, try to wait for recovery */ - if (ret == -EIO && postcopy_pause_fault_thread(mis)) { + if (postcopy_pause_fault_thread(mis)) { /* We got reconnected somehow, try to continue */ goto retry; } else {
We used to have quite a few places making sure -EIO happened and that's the only way to trigger postcopy recovery. That's based on the assumption that we'll only return -EIO for channel issues. It'll work in 99.99% cases but logically that won't cover some corner cases. One example is e.g. ram_block_from_stream() could fail with an interrupted network, then -EINVAL will be returned instead of -EIO. I remembered Dave Gilbert pointed that out before, but somehow this is overlooked. Neither did I encounter anything outside the -EIO error. However we'd better touch that up before it triggers a rare VM data loss during live migrating. To cover as much those cases as possible, remove the -EIO restriction on triggering the postcopy recovery, because even if it's not a channel failure, we can't do anything better than halting QEMU anyway - the corpse of the process may even be used by a good hand to dig out useful memory regions, or the admin could simply kill the process later on. Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.c | 4 ++-- migration/postcopy-ram.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)