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

[Vladimir Sementsov-Ogievskiy]

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 <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.

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