From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60724) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAbek-0002XQ-A4 for qemu-devel@nongnu.org; Wed, 08 Jun 2016 07:28:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bAbee-0008MB-7c for qemu-devel@nongnu.org; Wed, 08 Jun 2016 07:28:17 -0400 Date: Wed, 8 Jun 2016 07:28:09 -0400 (EDT) From: Paolo Bonzini Message-ID: <666270289.20792192.1465385289909.JavaMail.zimbra@redhat.com> In-Reply-To: <20160608093229.GC5324@noname.str.redhat.com> References: <20160606144212.24074-1-mreitz@redhat.com> <20160606144212.24074-3-mreitz@redhat.com> <20160608093229.GC5324@noname.str.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/3] block/mirror: Fix target backing BDS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Max Reitz , qemu-block@nongnu.org, qemu-devel@nongnu.org, Fam Zheng , nsoffer@redhat.com, eblake@redhat.com ----- 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. 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