Re: [Qemu-devel] [PATCH 1/2] block/raw-format: switch to BDRV_BLOCK_DATA with BDRV_BLOCK_RECURSE

[Kevin Wolf]

Am 12.08.2019 um 20:11 hat Vladimir Sementsov-Ogievskiy geschrieben:
> BDRV_BLOCK_RAW makes generic bdrv_co_block_status to fallthrough to
> returned file. But is it correct behavior at all? If returned file
> itself has a backing file, we may report as totally unallocated and
> area which actually has data in bottom backing file.
> 
> So, mirroring of qcow2 under raw-format is broken. Which is illustrated
> by following commit with a test. Let's make raw-format behave more
> correctly returning BDRV_BLOCK_DATA.
> 
> Suggested-by: Max Reitz <address@hidden>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>

After some reading, I think I came to the conclusion that RAW is the
correct thing to do. There is indeed a problem, but this patch is trying
to fix it in the wrong place.

In the case where the backing file contains some data, and we have a
'raw' node above the qcow2 overlay node, the content of the respective
block is not defined by the queried backing file layer, so it is
completely correct that bdrv_is_allocated() returns false, like it would
if you queried the qcow2 layer directly. If it returned true, we would
copy everything, which isn't right either (the test cases should may add
the qemu-img map output of the target so this becomes visible).

The problem is that we try to recurse along the backing chain, but we
fail to make the step from the raw node to the backing file.

Note that just extending Max's "deal with filters" is not enough to fix
this because raw doesn't actually meet all of the criteria for being a
filter in this sense (at least because the 'offset' option can change
offsets between raw and its child).

I think this is essentially a result of special-casing backing files
everywhere instead of treating them like children like any other.
bdrv_co_block_status_above() probably shouldn't recurse along the
backing chain, but along the returned *file pointers, and consider the
returned offset in *map.

Kevin