13.08.2019 14:04, Kevin Wolf wrote: > 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 <mre...@redhat.com> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > > 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.
I'd say, the problem is that we ignore backing chain of non-backing child > > 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. But we need to special-case them, as we have interfaces operating on backing chain, > 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. > So, you mean that in case of unallocated, format layer should return it's backing file as file? Then, maybe bdrv_co_block_status should not recurse at all? -- Best regards, Vladimir
