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

[Vladimir Sementsov-Ogievskiy]

13.08.2019 18:03, Max Reitz wrote:
> On 13.08.19 16:56, Vladimir Sementsov-Ogievskiy wrote:
>> 13.08.2019 17:43, Max Reitz wrote:
>>> On 13.08.19 13: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.
>>>
>>> I disagree.  The queried backing file layer is the raw node.  As I said,
>>> in my opinion raw nodes are not filter nodes, neither in behavior (they
>>> have an offset option), nor in how they are generally used (as a format).
>>>
>>> The raw format does not support backing files.  Therefore, everything on
>>> a raw node is allocated.
>>>
>>
>> Could you tell me at least, what means "allocated" ?
>>
>> It's a term that describing a region somehow.. But how? Allocated where?
>> In raw node, in its child or both? Am I right that if region allocated in
>> one of non-cow children it is assumed to be allocated in parent too? Or what?
>>
>> And it's unrelated to real disk allocation which (IMHO) directly shows that
>> this a bad term.
> 
> It’s a term for COW backing chains.  If something is allocated on a
> given node in a COW backing chain, it means it is either present in
> exactly that node or in one of its storage children (in case the node is
> a format node).  If it is not allocated, it is not, and read accesses
> will be forwarded to the COW backing child.
> 

And this definition leads exactly to bug in these series:


[raw]
   |
   |file
   V       file
[qcow2]--------->[file]
   |
   |backing
   V
[base]


Assume something is actually allocated in [base] but not in [qcow2].
So, [qcow2] node reports it as unallocated. So nobdy of [raw]'s storage
children contains this as allocated, so it's unallocated in [raw].


And if you say that logic is different for raw as it don't have COW child,
we may put qcow2 instead of raw here with same problem [yes, qcow2 on qcow2
is a bit strange, but we try to make something generic]




-- 
Best regards,
Vladimir