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

[Kevin Wolf]

Am 13.08.2019 um 14:01 hat Kevin Wolf geschrieben:
> Am 13.08.2019 um 13:28 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > 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
> 
> Yes, exactly. And I know even less about what happens if a child is
> neither bs->file nor bs->backing. Imagine a qcow2 image with an external
> data file that is a qcow2 image with a backing file itself. :-)
> 
> Actually, just having two qcow2 layers nested with bs->file probably
> already fails.
> 
> > > 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,
> 
> I'm not sure yet if this means that these interfaces are wrong, but it
> might. But in any case, I think we depend on special-casing in more
> places than we should.
> 
> > > 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?
> 
> Yes, because that's where it's reading the data from.
> 
> Hm... Now I wonder what this means for DATA... In theory it would have
> to be set for backing files, but that would make it completely useless.
> We can distinguish the cases by looking at *file, but how does the
> generic block layer know which child should be counted as "allocated"
> and which shouldn't?

Possible answer to my own question:

bdrv_is_allocated(bs) isn't even asking a complete question. What we
really need to ask is whether a specific child is where data comes from.

What the current callers of bdrv_is_allocated() are interested in is
whether the data comes from bs->backing or from somewhere else. That is,
if removing bs from the graph (so that all parents of bs would point to
bs->backing instead) would still result in the same data in the given
block.

Kevin