[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block/raw-format: switch to BD
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block/raw-format: switch to BDRV_BLOCK_DATA with BDRV_BLOCK_RECURSE |
Date: |
Tue, 13 Aug 2019 15:21:50 +0200 |
User-agent: |
Mutt/1.11.3 (2019-02-01) |
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
- [Qemu-devel] [PATCH 0/2] deal with BDRV_BLOCK_RAW, Vladimir Sementsov-Ogievskiy, 2019/08/12
- Re: [Qemu-devel] [PATCH 1/2] block/raw-format: switch to BDRV_BLOCK_DATA with BDRV_BLOCK_RECURSE, Max Reitz, 2019/08/13
- Re: [Qemu-devel] [PATCH 1/2] block/raw-format: switch to BDRV_BLOCK_DATA with BDRV_BLOCK_RECURSE, Vladimir Sementsov-Ogievskiy, 2019/08/13
- Re: [Qemu-devel] [PATCH 1/2] block/raw-format: switch to BDRV_BLOCK_DATA with BDRV_BLOCK_RECURSE, Max Reitz, 2019/08/13
- Re: [Qemu-devel] [PATCH 1/2] block/raw-format: switch to BDRV_BLOCK_DATA with BDRV_BLOCK_RECURSE, Vladimir Sementsov-Ogievskiy, 2019/08/13
- Re: [Qemu-devel] [PATCH 1/2] block/raw-format: switch to BDRV_BLOCK_DATA with BDRV_BLOCK_RECURSE, Max Reitz, 2019/08/13
- Re: [Qemu-devel] [PATCH 1/2] block/raw-format: switch to BDRV_BLOCK_DATA with BDRV_BLOCK_RECURSE, Kevin Wolf, 2019/08/13
- Re: [Qemu-devel] [PATCH 1/2] block/raw-format: switch to BDRV_BLOCK_DATA with BDRV_BLOCK_RECURSE, Vladimir Sementsov-Ogievskiy, 2019/08/13
- Re: [Qemu-devel] [PATCH 1/2] block/raw-format: switch to BDRV_BLOCK_DATA with BDRV_BLOCK_RECURSE, Kevin Wolf, 2019/08/13
- Re: [Qemu-devel] [PATCH 1/2] block/raw-format: switch to BDRV_BLOCK_DATA with BDRV_BLOCK_RECURSE, Vladimir Sementsov-Ogievskiy, 2019/08/13