13.08.2019 18:41, Kevin Wolf wrote: > Am 13.08.2019 um 16:43 hat Max Reitz geschrieben: >> 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 <mre...@redhat.com> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >>> >>> 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. >> >> (That is, like, my opinion.) >> >>> 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). >> >> It is right. > > So we don't even agree what mirroring the raw node should even mean. > > I can the see your point when you say that the raw node has no backing > file, so everything should be copied. But I can also see the point that > the raw node can really just be used as a filter that limits the data > exposed from the qcow2 layer, and you want to keep the copy a COW > overlay over the same backing file. > > Both are valid use cases in principle and there is no single right or > wrong. > > We don't currently support the latter use case because we have only > sync=full or sync=top, but if you could specify a base node instead, we > could probably suport the case without all of the special-casing filter > nodes and backing file childs. > > You would call bdrv_co_block_status_above() with the right base node and > it would just recurse whereever the data is stored, be it bs->backing, > bs->file or even driver-specific children. This would allow you to find > out whether some block in the top node came from the base node that > we're going to keep. If yes, skip it; if no, copy it. > > This way we wouldn't have to decide whether raw is a filter or not, > because it wouldn't make a difference. The behaviour would only depend > on the base node given by the user. If you specified the top-level qcow2 > file as the base, you get your full copy;
ahm, full-copy = base is NULL.. > if you specified the backing > qcow2, you get the partial copy where the target still uses the same > backing file. > > (Hm... It would only actually work if the offsets stay the same in the > chain, which is true for backing file children, but not necessarily for > other children. Don't follow, what you mean by offsets stay the same and what is wrong with it? > Anyway, even if we don't gain much functionality, I > really want a more generic model that avoids different types of nodes > and edges as much as possible.) > > Kevin > -- Best regards, Vladimir
