| Message ID | 1397205095-29830-1-git-send-email-mjt@msgid.tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
Quoting Michael Tokarev (mjt@tls.msk.ru): > ENOENT means the kernel has an empty dirty bitmap for this > slot. Don't abort in that case. This appears to solve > the bug reported at > > https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1303926 > > which first showed up with commit b533f658a98325d: fix return > check for KVM_GET_DIRTY_LOG ioctl > > Cc: Serge Hallyn <serge.hallyn@ubuntu.com> > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> This does look better than my version, thanks. As you say my version was closer to the old behavior, but if the kernel has no dirty bitmap we shouldn't be acting on it, so this looks better imo. Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com> (I've not tested yet, but it looks right and works for you :) > --- > kvm-all.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/kvm-all.c b/kvm-all.c > index cd4111d..47fa948 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -441,13 +441,19 @@ static int kvm_physical_sync_dirty_bitmap(MemoryRegionSection *section) > > d.slot = mem->slot; > > - if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) < 0) { > + ret = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d); > + if (ret >= 0) { > + /* regular case, process returned bitmap */ > + kvm_get_dirty_pages_log_range(section, d.dirty_bitmap); > + } else if (ret == -ENOENT) { > + /* kernel does not have dirty bitmap in this slot */ > + ret = 0; > + } else { > DPRINTF("ioctl failed %d\n", errno); > ret = -1; > break; > } > > - kvm_get_dirty_pages_log_range(section, d.dirty_bitmap); > start_addr = mem->start_addr + mem->memory_size; > } > g_free(d.dirty_bitmap); > -- > 1.7.10.4 >
Il 11/04/2014 04:31, Michael Tokarev ha scritto: > ENOENT means the kernel has an empty dirty bitmap for this > slot. Don't abort in that case. This appears to solve > the bug reported at > > https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1303926 > > which first showed up with commit b533f658a98325d: fix return > check for KVM_GET_DIRTY_LOG ioctl > > Cc: Serge Hallyn <serge.hallyn@ubuntu.com> > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > --- > kvm-all.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/kvm-all.c b/kvm-all.c > index cd4111d..47fa948 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -441,13 +441,19 @@ static int kvm_physical_sync_dirty_bitmap(MemoryRegionSection *section) > > d.slot = mem->slot; > > - if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) < 0) { > + ret = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d); > + if (ret >= 0) { > + /* regular case, process returned bitmap */ > + kvm_get_dirty_pages_log_range(section, d.dirty_bitmap); > + } else if (ret == -ENOENT) { > + /* kernel does not have dirty bitmap in this slot */ > + ret = 0; > + } else { > DPRINTF("ioctl failed %d\n", errno); > ret = -1; > break; > } > > - kvm_get_dirty_pages_log_range(section, d.dirty_bitmap); > start_addr = mem->start_addr + mem->memory_size; > } > g_free(d.dirty_bitmap); > I'd rather revert b533f658a98325d instead. Paolo
Quoting Paolo Bonzini (pbonzini@redhat.com): > Il 11/04/2014 04:31, Michael Tokarev ha scritto: > >ENOENT means the kernel has an empty dirty bitmap for this > >slot. Don't abort in that case. This appears to solve > >the bug reported at > > > >https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1303926 > > > >which first showed up with commit b533f658a98325d: fix return > >check for KVM_GET_DIRTY_LOG ioctl > > > >Cc: Serge Hallyn <serge.hallyn@ubuntu.com> > >Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > >--- > > kvm-all.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > >diff --git a/kvm-all.c b/kvm-all.c > >index cd4111d..47fa948 100644 > >--- a/kvm-all.c > >+++ b/kvm-all.c > >@@ -441,13 +441,19 @@ static int kvm_physical_sync_dirty_bitmap(MemoryRegionSection *section) > > > > d.slot = mem->slot; > > > >- if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) < 0) { > >+ ret = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d); > >+ if (ret >= 0) { > >+ /* regular case, process returned bitmap */ > >+ kvm_get_dirty_pages_log_range(section, d.dirty_bitmap); > >+ } else if (ret == -ENOENT) { > >+ /* kernel does not have dirty bitmap in this slot */ > >+ ret = 0; > >+ } else { > > DPRINTF("ioctl failed %d\n", errno); > > ret = -1; > > break; > > } > > > >- kvm_get_dirty_pages_log_range(section, d.dirty_bitmap); > > start_addr = mem->start_addr + mem->memory_size; > > } > > g_free(d.dirty_bitmap); > > > > I'd rather revert b533f658a98325d instead. That seems wrong though. If we want to ignore all errors that's one thing, but before that commit we just ignored all errors other than EPERM.
Il 11/04/2014 22:10, Serge Hallyn ha scritto: > Quoting Paolo Bonzini (pbonzini@redhat.com): >> Il 11/04/2014 04:31, Michael Tokarev ha scritto: >>> ENOENT means the kernel has an empty dirty bitmap for this >>> slot. Don't abort in that case. This appears to solve >>> the bug reported at >>> >>> https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1303926 >>> >>> which first showed up with commit b533f658a98325d: fix return >>> check for KVM_GET_DIRTY_LOG ioctl >>> >>> Cc: Serge Hallyn <serge.hallyn@ubuntu.com> >>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> >>> --- >>> kvm-all.c | 10 ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/kvm-all.c b/kvm-all.c >>> index cd4111d..47fa948 100644 >>> --- a/kvm-all.c >>> +++ b/kvm-all.c >>> @@ -441,13 +441,19 @@ static int kvm_physical_sync_dirty_bitmap(MemoryRegionSection *section) >>> >>> d.slot = mem->slot; >>> >>> - if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) < 0) { >>> + ret = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d); >>> + if (ret >= 0) { >>> + /* regular case, process returned bitmap */ >>> + kvm_get_dirty_pages_log_range(section, d.dirty_bitmap); >>> + } else if (ret == -ENOENT) { >>> + /* kernel does not have dirty bitmap in this slot */ >>> + ret = 0; >>> + } else { >>> DPRINTF("ioctl failed %d\n", errno); >>> ret = -1; >>> break; >>> } >>> >>> - kvm_get_dirty_pages_log_range(section, d.dirty_bitmap); >>> start_addr = mem->start_addr + mem->memory_size; >>> } >>> g_free(d.dirty_bitmap); >>> >> >> I'd rather revert b533f658a98325d instead. > > That seems wrong though. If we want to ignore all errors that's one > thing, but before that commit we just ignored all errors other than > EPERM. It is wrong, and the patch would get in again for 2.1 via the KVM tree. But if the patch caused a regression it obviously wasn't trivial enough, and so close to 2.0 the safest thing to do is revert it. Paolo
Quoting Paolo Bonzini (pbonzini@redhat.com): > Il 11/04/2014 22:10, Serge Hallyn ha scritto: > >Quoting Paolo Bonzini (pbonzini@redhat.com): > >>Il 11/04/2014 04:31, Michael Tokarev ha scritto: > >>>ENOENT means the kernel has an empty dirty bitmap for this > >>>slot. Don't abort in that case. This appears to solve > >>>the bug reported at > >>> > >>>https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1303926 > >>> > >>>which first showed up with commit b533f658a98325d: fix return > >>>check for KVM_GET_DIRTY_LOG ioctl > >>> > >>>Cc: Serge Hallyn <serge.hallyn@ubuntu.com> > >>>Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > >>>--- > >>>kvm-all.c | 10 ++++++++-- > >>>1 file changed, 8 insertions(+), 2 deletions(-) > >>> > >>>diff --git a/kvm-all.c b/kvm-all.c > >>>index cd4111d..47fa948 100644 > >>>--- a/kvm-all.c > >>>+++ b/kvm-all.c > >>>@@ -441,13 +441,19 @@ static int kvm_physical_sync_dirty_bitmap(MemoryRegionSection *section) > >>> > >>> d.slot = mem->slot; > >>> > >>>- if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) < 0) { > >>>+ ret = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d); > >>>+ if (ret >= 0) { > >>>+ /* regular case, process returned bitmap */ > >>>+ kvm_get_dirty_pages_log_range(section, d.dirty_bitmap); > >>>+ } else if (ret == -ENOENT) { > >>>+ /* kernel does not have dirty bitmap in this slot */ > >>>+ ret = 0; > >>>+ } else { > >>> DPRINTF("ioctl failed %d\n", errno); > >>> ret = -1; > >>> break; > >>> } > >>> > >>>- kvm_get_dirty_pages_log_range(section, d.dirty_bitmap); > >>> start_addr = mem->start_addr + mem->memory_size; > >>> } > >>> g_free(d.dirty_bitmap); > >>> > >> > >>I'd rather revert b533f658a98325d instead. > > > >That seems wrong though. If we want to ignore all errors that's one > >thing, but before that commit we just ignored all errors other than > >EPERM. > > It is wrong, and the patch would get in again for 2.1 via the KVM > tree. But if the patch caused a regression it obviously wasn't > trivial enough, and so close to 2.0 the safest thing to do is revert > it. True it seemed all right before that commit. Though if it wasn't all right we'd see the error a long ways down the road. The only other errors we expect from the ioctl are EFAULT and EINVAL. EINVAL would mean we fed it bad data (log->slot >= KVM_USER_MEM_SLOTS), while EFAULT would mean we passed a bad address for the returned bitmap. The latter one probably shouldn't be ignored - however it's not as though we do anything in the error case to try and recover. So given that the code has been as it was since at least v1.3.0, perhaps a revert is the most responsible thing to do. -serge
diff --git a/kvm-all.c b/kvm-all.c index cd4111d..47fa948 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -441,13 +441,19 @@ static int kvm_physical_sync_dirty_bitmap(MemoryRegionSection *section) d.slot = mem->slot; - if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) < 0) { + ret = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d); + if (ret >= 0) { + /* regular case, process returned bitmap */ + kvm_get_dirty_pages_log_range(section, d.dirty_bitmap); + } else if (ret == -ENOENT) { + /* kernel does not have dirty bitmap in this slot */ + ret = 0; + } else { DPRINTF("ioctl failed %d\n", errno); ret = -1; break; } - kvm_get_dirty_pages_log_range(section, d.dirty_bitmap); start_addr = mem->start_addr + mem->memory_size; } g_free(d.dirty_bitmap);
ENOENT means the kernel has an empty dirty bitmap for this slot. Don't abort in that case. This appears to solve the bug reported at https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1303926 which first showed up with commit b533f658a98325d: fix return check for KVM_GET_DIRTY_LOG ioctl Cc: Serge Hallyn <serge.hallyn@ubuntu.com> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- kvm-all.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)