On 08.06.2016 16:40, Max Reitz wrote: > On 08.06.2016 13:28, Paolo Bonzini wrote: >> >> >> ----- Original Message ----- >>> From: "Kevin Wolf" >>> To: "Max Reitz" >>> Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, "Fam Zheng" , nsoffer@redhat.com, >>> eblake@redhat.com, pbonzini@redhat.com >>> Sent: Wednesday, June 8, 2016 11:32:29 AM >>> Subject: Re: [PATCH v2 2/3] block/mirror: Fix target backing BDS >>> >>> Am 06.06.2016 um 16:42 hat Max Reitz geschrieben: >>>> Currently, we are trying to move the backing BDS from the source to the >>>> target in bdrv_replace_in_backing_chain() which is called from >>>> mirror_exit(). However, mirror_complete() already tries to open the >>>> target's backing chain with a call to bdrv_open_backing_file(). >>>> >>>> First, we should only set the target's backing BDS once. Second, the >>>> mirroring block job has a better idea of what to set it to than the >>>> generic code in bdrv_replace_in_backing_chain() (in fact, the latter's >>>> conditions on when to move the backing BDS from source to target are not >>>> really correct). >>>> >>>> Therefore, remove that code from bdrv_replace_in_backing_chain() and >>>> leave it to mirror_complete(). >>>> >>>> However, mirror_complete() in turn pursues a questionable strategy by >>>> employing bdrv_open_backing_file(): On the one hand, because this may >>>> open the wrong backing file with drive-mirror in "existing" mode, or >>>> because it will not override a possibly wrong backing file in the >>>> blockdev-mirror case. >>> >>> Careful, this "wrong" backing file might actually be intended! >>> >>> Consider a case where you want to move an image with its whole backing >>> chain to different storage. In that case, you would copy all of the >>> backing files (cp is good enough, they are read-only), create the >>> destination image which already points at the copied backing chain, and >>> then mirror in "existing" mode. >>> >>> The intention is obviously that after the job completion the new backing >>> chain is used and not the old one. >> >> Yes, this is the intention and it should not be changed. In addition >> to what Kevin said, you can use drive-mirror to collapse the image to a >> single file; in this case, QEMU should not be using the backing files of >> the source. > > That is an issue that we have right now. If you do drive-mirror in > absolute-paths mode with sync=full, the target will have the backing > chain of the source. This is something that this patch fixes. As a clarification: I mean the backing chain inside QEMU (in the BDS graph), not the on-disk backing chain, i.e. how the physical image files link to each other. Max > In fact, I think if you do drive-mirror in existing mode or > blockdev-mirror and the target image does not have a backing file > (whatever sync mode you have used), the same will happen. > > Max > >> bdrv_open_backing_file() is used because what we want to do is to >> "undo" the BDRV_O_NO_BACKING flag used by qmp_drive_mirror. >> >> If the contents change under the guest feet, it's the layers above >> QEMU that have screwed up. >> >> Paolo >> > >