Eric Blake
2021-Apr-12 22:06 UTC
[Libguestfs] [libnbd PATCH] interop: Tolerate qemu-nbd 6.0 change in behavior
Qemu 6.0 commit 0da9856851 (nbd: server: Report holes for faw images)
intentionally changed how holes are reported, not only for raw images,
but for known-zero portions of qcow2 files. As the point of our
dirty-bitmap test is more about reading multiple contexts from a
single qemu-nbd process, it is okay to accept both behaviors for
base:allocation reporting on a block of zeros.
---
interop/dirty-bitmap.c | 40 +++++++++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 11 deletions(-)
diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c
index 5f9fa12f..ed445712 100644
--- a/interop/dirty-bitmap.c
+++ b/interop/dirty-bitmap.c
@@ -1,5 +1,5 @@
/* NBD client library in userspace
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2021 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -48,7 +48,10 @@ cb (void *opaque, const char *metacontext, uint64_t offset,
/* libnbd does not actually verify that a server is fully compliant
* to the spec; the asserts marked [qemu-nbd] are thus dependent on
- * the fact that qemu-nbd is compliant.
+ * the fact that qemu-nbd is compliant. Furthermore, qemu 5.2 and
+ * 6.0 disagree on whether base:allocation includes the hole bit for
+ * the zeroes at 512k (both answers are compliant); but we care more
+ * that the zeroes show up in the dirty bitmap
*/
assert (offset == 0);
assert (!*error || (data->fail && data->count == 1 &&
*error == EPROTO));
@@ -57,19 +60,34 @@ cb (void *opaque, const char *metacontext, uint64_t offset,
if (strcmp (metacontext, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0) {
assert (!data->seen_base); /* [qemu-nbd] */
data->seen_base = true;
- assert (len == (data->req_one ? 2 : 8)); /* [qemu-nbd] */
+ if (data->req_one)
+ assert (len == 2); /* [qemu-nbd] */
+ else
+ assert ((len & 1) == 0 && len > 2); /* [qemu-nbd] */
/* Data block offset 0 size 128k */
assert (entries[0] == 131072); assert (entries[1] == 0);
if (!data->req_one) {
- /* hole|zero offset 128k size 384k */
- assert (entries[2] == 393216); assert (entries[3] == (LIBNBD_STATE_HOLE|
-
LIBNBD_STATE_ZERO));
- /* allocated zero offset 512k size 64k */
- assert (entries[4] == 65536); assert (entries[5] == LIBNBD_STATE_ZERO);
- /* hole|zero offset 576k size 448k */
- assert (entries[6] == 458752); assert (entries[7] == (LIBNBD_STATE_HOLE|
-
LIBNBD_STATE_ZERO));
+ if (len == 4) {
+ /* hole|zero offset 128k size 896k */
+ assert (entries[2] == 917504);
+ assert (entries[3] == (LIBNBD_STATE_HOLE|
+ LIBNBD_STATE_ZERO));
+ }
+ else {
+ assert (len == 8);
+ /* hole|zero offset 128k size 384k */
+ assert (entries[2] == 393216);
+ assert (entries[3] == (LIBNBD_STATE_HOLE|
+ LIBNBD_STATE_ZERO));
+ /* allocated zero offset 512k size 64k */
+ assert (entries[4] == 65536);
+ assert (entries[5] == LIBNBD_STATE_ZERO);
+ /* hole|zero offset 576k size 448k */
+ assert (entries[6] == 458752);
+ assert (entries[7] == (LIBNBD_STATE_HOLE|
+ LIBNBD_STATE_ZERO));
+ }
}
}
else if (strcmp (metacontext, bitmap) == 0) {
--
2.31.1
Richard W.M. Jones
2021-Apr-13 07:33 UTC
[Libguestfs] [libnbd PATCH] interop: Tolerate qemu-nbd 6.0 change in behavior
I tested this with: - upstream qemu - upstream qemu with Nir's patch reverted - qemu 5.2 and it worked in all of those cases. Pushed it now, thanks. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Nir Soffer
2021-Apr-13 16:33 UTC
[Libguestfs] [libnbd PATCH] interop: Tolerate qemu-nbd 6.0 change in behavior
On Tue, Apr 13, 2021 at 1:07 AM Eric Blake <eblake at redhat.com> wrote:> > Qemu 6.0 commit 0da9856851 (nbd: server: Report holes for faw images)raw> intentionally changed how holes are reported, not only for raw images, > but for known-zero portions of qcow2 files.Do you have an example for known-zero?> As the point of our > dirty-bitmap test is more about reading multiple contexts from a > single qemu-nbd process, it is okay to accept both behaviors for > base:allocation reporting on a block of zeros. > --- > interop/dirty-bitmap.c | 40 +++++++++++++++++++++++++++++----------- > 1 file changed, 29 insertions(+), 11 deletions(-) > > diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c > index 5f9fa12f..ed445712 100644 > --- a/interop/dirty-bitmap.c > +++ b/interop/dirty-bitmap.c > @@ -1,5 +1,5 @@ > /* NBD client library in userspace > - * Copyright (C) 2013-2019 Red Hat Inc. > + * Copyright (C) 2013-2021 Red Hat Inc. > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public > @@ -48,7 +48,10 @@ cb (void *opaque, const char *metacontext, uint64_t offset, > > /* libnbd does not actually verify that a server is fully compliant > * to the spec; the asserts marked [qemu-nbd] are thus dependent on > - * the fact that qemu-nbd is compliant. > + * the fact that qemu-nbd is compliant. Furthermore, qemu 5.2 and > + * 6.0 disagree on whether base:allocation includes the hole bit for > + * the zeroes at 512k (both answers are compliant); but we care more > + * that the zeroes show up in the dirty bitmapSo 6.0 show reports a hole for zeroed area, and 5.2 reports data? When we copy the image with qemu-img convert, do we get a hole in the destination?> */ > assert (offset == 0); > assert (!*error || (data->fail && data->count == 1 && *error == EPROTO)); > @@ -57,19 +60,34 @@ cb (void *opaque, const char *metacontext, uint64_t offset, > if (strcmp (metacontext, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0) { > assert (!data->seen_base); /* [qemu-nbd] */ > data->seen_base = true; > - assert (len == (data->req_one ? 2 : 8)); /* [qemu-nbd] */ > + if (data->req_one) > + assert (len == 2); /* [qemu-nbd] */ > + else > + assert ((len & 1) == 0 && len > 2); /* [qemu-nbd] */ > > /* Data block offset 0 size 128k */ > assert (entries[0] == 131072); assert (entries[1] == 0); > if (!data->req_one) { > - /* hole|zero offset 128k size 384k */ > - assert (entries[2] == 393216); assert (entries[3] == (LIBNBD_STATE_HOLE| > - LIBNBD_STATE_ZERO)); > - /* allocated zero offset 512k size 64k */ > - assert (entries[4] == 65536); assert (entries[5] == LIBNBD_STATE_ZERO); > - /* hole|zero offset 576k size 448k */ > - assert (entries[6] == 458752); assert (entries[7] == (LIBNBD_STATE_HOLE| > - LIBNBD_STATE_ZERO)); > + if (len == 4) { > + /* hole|zero offset 128k size 896k */Is this the qemu-nbd 5.2 behavior?> + assert (entries[2] == 917504); > + assert (entries[3] == (LIBNBD_STATE_HOLE| > + LIBNBD_STATE_ZERO)); > + } > + else { > + assert (len == 8); > + /* hole|zero offset 128k size 384k */Moving the comment under the else will be more clear.> + assert (entries[2] == 393216); > + assert (entries[3] == (LIBNBD_STATE_HOLE| > + LIBNBD_STATE_ZERO)); > + /* allocated zero offset 512k size 64k */ > + assert (entries[4] == 65536); > + assert (entries[5] == LIBNBD_STATE_ZERO); > + /* hole|zero offset 576k size 448k */ > + assert (entries[6] == 458752); > + assert (entries[7] == (LIBNBD_STATE_HOLE| > + LIBNBD_STATE_ZERO)); > + } > } > } > else if (strcmp (metacontext, bitmap) == 0) { > -- > 2.31.1 >