Bug 1968693

Eric, can you have a look at this one?

I posted experimental fix upstream:
https://lists.nongnu.org/archive/html/qemu-block/2021-06/msg00347.html

This fixes RHV issue, but it is not complete and correct for all
cases, but there is discussion on the right way to solve this issue.

I posted a counter-proposal patch that makes it easier to get at the already-existing information in qemu:
https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg02446.html

But note that such a patch requires a change in ovirt to consume the new context; and there is still more work to be done before qemu-img map can access the new information.  nbdinfo can easily list it, but even that project would need a patch to expose human-readable names of the bits.

So the upstream solution requires quite a few downstream backports across multiple projects, vs. a simpler choice of a downstream-only revert in just qemu of the patch that changed qemu 6.0 behavior.  But note that a downstream revert, while feasible in the short term, will not be perpetuated indefinitely, so ultimately ovirt will need to start paying attention to either qemu:allocation-depth or the proposed qemu:joint-allocation, where a qemu revert is only an option for qemu versions that must be used with unpatched ovirt.

(In reply to Eric Blake from comment #3)
> But note that such a patch requires a change in ovirt to consume the new
> context; 

This is fine, I started to work on this here:
https://gerrit.ovirt.org/c/ovirt-imageio/+/115197

We can consume the new context in next ovirt release - 4.4.7.

> and there is still more work to be done before qemu-img map can
> access the new information.  nbdinfo can easily list it, but even that
> project would need a patch to expose human-readable names of the bits.

We don't depend on qemu-img map or nbdinfo, so this is fine. 

> So the upstream solution requires quite a few downstream backports across
> multiple projects, vs. a simpler choice of a downstream-only revert in just
> qemu of the patch that changed qemu 6.0 behavior.  But note that a
> downstream revert, while feasible in the short term, will not be perpetuated
> indefinitely, so ultimately ovirt will need to start paying attention to
> either qemu:allocation-depth or the proposed qemu:joint-allocation, where a
> qemu revert is only an option for qemu versions that must be used with
> unpatched ovirt.

I think we must revert the original patch downstream regardless of the new
context, since it breaks existing code, and the breakage is the worst kind,
silent data corruption during backup.

Reverting the patch will also give us more time work on a better long term
solution.

We will be happy to consume the new context in qemu 6.0, but we can also
wait to 6.1 if this work needs more time.

RHV-4.4.7 will resolve this issue by using qemu:allocation-depth, see bug 1971182.

Can reproduce this issue,
Version:
qemu-kvm-6.0.0-19.module+el8.5.0+11385+6e7d542e.x86_64
kernel-4.18.0-310.el8.x86_64

Steps:
# Create an image with a backing file:
$ qemu-img create base.raw $((4*64*1024))
$ qemu-img create -f qcow2 -b base.raw -F raw top.qcow2

# Write data to the first 3 cluster in the base image:
$ qemu-io -f raw -c "write -P 65 0 64k" base.raw
$ qemu-io -f raw -c "write -P 66 64k 64k" base.raw
$ qemu-io -f raw -c "write -P 67 128k 64k" base.raw

# Write data to the second cluster of the top image, hiding the second cluster in the backing file:
$ qemu-io -f qcow2 -c "write -P 69 64k 64k" top.qcow2

# Write zeroes to the third cluster of the top imge, hiding the third cluster in the backing file:
$ qemu-io -f qcow2 -c "write -z 128k 64k" top.qcow2



# Query the image via qemu-nbd

$ qemu-nbd -r -t -f qcow2 top.qcow2 &

$ qemu-img map --output json nbd://localhost
[{ "start": 0, "length": 131072, "depth": 0, "zero": false, "data": true, "offset": 0},
{ "start": 131072, "length": 131072, "depth": 0, "zero": true, "data": false, "offset": 131072}]

$ nbdinfo --map nbd://localhost
         0      131072    0  allocated
    131072      131072    3  hole,zero

Expected result:
should not report a hole.

Seems like the upstream discussion converged? Can we expect a POST soon?

(In reply to Klaus Heinrich Kiwi from comment #7)
> Seems like the upstream discussion converged? Can we expect a POST soon?

Eric, any updates?

(In reply to Nir Soffer from comment #5)
> RHV-4.4.7 will resolve this issue by using qemu:allocation-depth, see bug
> 1971182.

Nir,

 this fix was merged upstream (commit 8417e1378c) and will get that on 8.6 rebase. We haven't backported it yet for 8.5. Based on RHV changes, do we still need it?

(In reply to Klaus Heinrich Kiwi from comment #9)
> (In reply to Nir Soffer from comment #5)
> > RHV-4.4.7 will resolve this issue by using qemu:allocation-depth, see bug
> > 1971182.
>  this fix was merged upstream (commit 8417e1378c) and will get that on 8.6
> rebase. We haven't backported it yet for 8.5. Based on RHV changes, do we
> still need it?

I don't think this was fixed, what was fixed was reporting holes in a better
way in qemu-img map, but we don't use it, we use qemu-nbd, and it was not 
fixed since the change is considered a bug fix.

But RHV is using now allocation depth reported by qemu-nbd since 5.2.0, so
this bug does not affect us now (bug 1971182).

The only issue is with older RHV versions (< 4.4.7) that may get wrong
info from qemu-nbd >= 6. The only way to avoid the issue is to revert the 
change in rhel 8.5, but I'm not sure it worth the effort since users can
and should upgrade to RHV >= 4.4.7 anyway.

So I would close this bug as wontfix.

(In reply to Nir Soffer from comment #10)

> So I would close this bug as wontfix.

Thanks for the clarification, Nir.