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

On 4/13/21 11:33 AM, Nir Soffer wrote: > On Tue, Apr 13, 2021 at 1:07 AM Eric Blake <eblake(a)redhat.com&gt; 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? The libnbd input was easy enough to recreate (here, eliding the dirty-bitmap tracking that also factored into the test): qemu-img create -f qcow2 d.qcow2 1M qemu-io -f qcow2 -c 'w 0 64k' d.qcow2 qemu-io -f qcow2 -c 'w 64k 64k' -c 'w -z 512k 64k' d.qcow2 Looking at that file directly shows that the just-written zeroes at 512k are no different from the unallocated zeroes: qemu-img map --output=json -f qcow2 d.qcow2 [{ "start": 0, "length": 131072, "depth": 0, "zero": false, "data": true, "offset": 327680}, { "start": 131072, "length": 917504, "depth": 0, "zero": true, "data": false}] But before your qemu patch, qemu-nbd reported things differently: old/qemu-nbd -f qcow2 d.qcow2& qemu-img map --output=json -f raw nbd://localhost:10809 [{ "start": 0, "length": 131072, "depth": 0, "zero": false, "data": true, "offset": 0}, { "start": 131072, "length": 393216, "depth": 0, "zero": true, "data": false, "offset": 131072}, { "start": 524288, "length": 65536, "depth": 0, "zero": true, "data": true, "offset": 524288}, { "start": 589824, "length": 458752, "depth": 0, "zero": true, "data": false, "offset": 589824}] [1]+ Done ~/qemu/qemu-nbd -f qcow2 d.qcow2 Compared to with your patch: qemu-nbd -f qcow2 d.qcow2& qemu-img map --output=json -f raw nbd://localhost:10809 [{ "start": 0, "length": 131072, "depth": 0, "zero": false, "data": true, "offset": 0}, { "start": 131072, "length": 917504, "depth": 0, "zero": true, "data": false, "offset": 131072}] [1]+ Done qemu-nbd -f qcow2 d.qcow2 So your patch fixed qemu-nbd to advertise status more consistent with what qemu-img map already sees for native qcow2 files.

>> @@ -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 > > So 6.0 show reports a hole for zeroed area, and 5.2 reports data? Yes. The qcow2 file is sparse (the qemu-io action did not allocate a cluster, but merely marked the cluster as explicit reads-as-zero). Both pre- and post-patch, we still know that reading at 512k will see zeroes. The only difference is whether we report if those zeroes are sparse (with 5.2, you might try to allocate more than necessary because the zeroes don't look sparse, in 6.0, they are correctly reported as sparse). But knowing sparseness is merely an optimization on whether to allocate at the destination, and not a correctness issue on what will be read at that location. > > When we copy the image with qemu-img convert, do we get > a hole in the destination? Easy enough to find out: Direct preserves the hole: $ qemu-img convert -f qcow2 -O qcow2 d.qcow2 e.qcow2 $ qemu-img map --output=json -f qcow2 e.qcow2 [{ "start": 0, "length": 131072, "depth": 0, "zero": false, "data": true, "offset": 327680}, { "start": 131072, "length": 917504, "depth": 0, "zero": true, "data": false}] Old qemu-nbd does not allocate zeroes by default, thereby giving the same sparse result as direct access: $ old/qemu-nbd -f qcow2 d.qcow2 & $ qemu-img convert -f raw -O qcow2 nbd://localhost:10809 f.qcow2 $ qemu-img map --output=json -f qcow2 f.qcow2 [{ "start": 0, "length": 131072, "depth": 0, "zero": false, "data": true, "offset": 327680}, { "start": 131072, "length": 917504, "depth": 0, "zero": true, "data": false}] and new qemu-nbd is indistinguishable from direct access: $ qemu-nbd -f qcow2 d.qcow2 & $ qemu-img convert -f raw -O qcow2 nbd://localhost:10809 g.qcow2 $ qemu-img map --output=json -f qcow2 g.qcow2 [{ "start": 0, "length": 131072, "depth": 0, "zero": false, "data": true, "offset": 327680}, { "start": 131072, "length": 917504, "depth": 0, "zero": true, "data": false}] >> /* 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? No, this branch is 6.0 behavior, > >> + assert (entries[2] == 917504); >> + assert (entries[3] == (LIBNBD_STATE_HOLE| >> + LIBNBD_STATE_ZERO)); >> + } >> + else { >> + assert (len == 8); while this branch is 5.2 behavior >> + /* 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 */ Except that moving the comment placement would then be less consistent with the other comments each explaining a pair of entries.

>> + 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 >> > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org