[Libguestfs] [libnbd PATCH] interop: Tolerate qemu-nbd 6.0 change in behavior
Nir Soffer
nsoffer at redhat.com
Wed Apr 14 22:30:34 UTC 2021
On Wed, Apr 14, 2021 at 12:36 AM Eric Blake <eblake at redhat.com> wrote:
>
> On 4/13/21 11:33 AM, Nir Soffer wrote:
> > 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?
>
> 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.
Thanks for the detailed example. This makes the issue very clear.
I wish the actual test was clear like these examples.
>
> >> @@ -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.
I think this test should be rewritten to more similar to qemu test 241:
https://github.com/qemu/qemu/blob/master/tests/qemu-iotests/241.out
Looking at json or simple text output makes it really easy to understand
what's going on.
Or have tow static lists or expected results:
old_base_alloc = {
{x, 0},
{y, zero | hole},
}
new_base_alloc = {
...
}
And use a generic compare helper to check that we got one of the
expected results.
> >> + 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
>
More information about the Libguestfs
mailing list