4:36 p.m.
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>
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