[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [libnbd PATCH] interop: Tolerate qemu-nbd 6.0 change in behavior



On Wed, Apr 14, 2021 at 12:36 AM Eric Blake <eblake 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 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
>


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]