Re: [Qemu-devel] [PATCH 0/2] deal with BDRV_BLOCK_RAW

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3638 bytes --]

On 13.08.19 11:34, Kevin Wolf wrote:
> Am 12.08.2019 um 21:46 hat Max Reitz geschrieben:
>> On 12.08.19 20:11, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> I'm not sure, is it a bug or a feature, but using qcow2 under raw is
>>> broken. It should be either fixed like I propose (by Max's suggestion)
>>> or somehow forbidden (just forbid backing-file supporting node to be
>>> file child of raw-format node).
>>
>> I agree, I think only filters should return BDRV_BLOCK_RAW.
> 
> If BDRV_BLOCK_RAW isn't suitable for raw, something went wrong. :-)

Yes, its introduction.

> But anyway, raw is essentially a filter, at least if you don't use
> the offset option.

Which is precisely why it isn’t a filter.

Another reason is that it generally appears as a replacement for a
format driver.  You never insert it into some graph because you want it
as a filter.  Which is why behaving like a format driver in block_status
makes sense, in my opinion.

>                    And BDRV_BLOCK_RAW shouldn't even depend on an
> unchanged offset because the .bdrv_co_block_status implementation
> returns the right offset.
> 
> And indeed, you can replace raw with blkdebug and it still fails (the
> testcase from patch 2 fails, too, but it's obvious enough this way):
> 
>     $ ./qemu-img map --output=json --image-opts driver=qcow2,file.driver=file,file.filename=/tmp/test.qcow2 
>     [{ "start": 0, "length": 1048576, "depth": 1, "zero": true, "data": false},
>     { "start": 1048576, "length": 1048576, "depth": 1, "zero": false, "data": true, "offset": 327680},
>     { "start": 2097152, "length": 65011712, "depth": 1, "zero": true, "data": false}]
> 
>     $ ./qemu-img map --output=json --image-opts driver=raw,file.driver=qcow2,file.file.driver=file,file.file.filename=/tmp/test.qcow2 
>     [{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": false}]
> 
>     $ ./qemu-img map --output=json --image-opts driver=blkdebug,image.driver=qcow2,image.file.driver=file,image.file.filename=/tmp/test.qcow2 
>     [{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": false}]
> 
> After applying your "deal with filters" series, blkdebug actually prints
> the expected result and passes the iotests case, but raw still doesn't.
> So you must have fixed something for filters that declare themselves
> filters,

I hope to have fixed many things for filters that declare themselves
filters. ;-)

I think the problem is that filters just break backing chains right now.
 My series fixes that.  (So even if you have a blkdebug node on top of
your qcow2 node, you should realize that the node you really care about
is the qcow2 node.  If you look at the filter, you won’t see the backing
file and will thus interpret unallocated areas the wrong way.)

(The most important problem is that bdrv_is_allocated_above() currently
only goes to the backing_bs().  That’s fixed by patch 13, “block: Use
CAFs in block status functions”.)

>          even though that semantics should probably be coupled to
> BDRV_BLOCK_RAW instead so that the raw format can benefit from it, too.

I think BDRV_BLOCK_RAW should just be dropped.  I don’t see its purpose
for non-filters now that we have BDRV_BLOCK_RECURSE; and filters should
just be handled generically in bdrv_co_block_status().

Alternatively, we can decide that calling block_status on a filter node
is a bad idea, because the caller may not be prepared for the fact that
block_status will not return information for this node.  But that would
mean dropping BDRV_BLOCK_RAW, too.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]