From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35423) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxbdK-0008R2-Dx for qemu-devel@nongnu.org; Wed, 27 May 2015 09:44:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YxbdF-0001eH-NC for qemu-devel@nongnu.org; Wed, 27 May 2015 09:44:34 -0400 Message-ID: <5565CA2A.1070409@redhat.com> Date: Wed, 27 May 2015 15:44:10 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1431105726-3682-1-git-send-email-kwolf@redhat.com> <1431105726-3682-12-git-send-email-kwolf@redhat.com> <555D7183.8080203@cn.fujitsu.com> <20150527123118.GE4669@noname.str.redhat.com> <20150527133035.GF4669@noname.str.redhat.com> In-Reply-To: <20150527133035.GF4669@noname.str.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 11/34] block: Allow references for backing files List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , Wen Congyang Cc: armbru@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com On 27/05/2015 15:30, Kevin Wolf wrote: > So it would be even more important to figure out what to do with > bdrv_swap(). > > Paolo, you added that bunch of assertions to bdrv_swap() when you > introduced it in commit 4ddc07ca. Did you just add them because they > were true at the time, or is anything actually relying on these > invariants? When I added bdrv_swap, the idea was to bs_new was either being closed afterwards, or (for bdrv_append) it would be the backing file of bs_top. In either case, the assertions would make sure that nothing would break when further manipulating bs_new. Are they being relied on? Probably not, but some violations would be just weird, such as a device attached to a non-topmost BDS, or a job attached to the old version of a mirrored disk. > What do we actually want to happen? Let's assume a small chain of > backing files: > > +---------- A-BlockBackend > | > | +----- B-BlockBackend > | | > base <- A <- B > > After completing the commit operation, what we want to have for the BB > of B is clear: > > +---------- B-BlockBackend > | > base <- A > > If we just remove the assertions that are currently present in > bdrv_swap(), I think we'd end up with this: > > +----- A-BlockBackend > | > +---------- B-BlockBackend > | | > base <- A <- B > > This is probably surprising for the user if they ever look at > A-BlockBackend again. It's also surprising because (unlike the case > without a BB for A) B is actually still referenced and therefore the > file stays opened. > > I suspect what we really want is this (which is not swapping): > > +---------- A-BlockBackend > | > +---------- B-BlockBackend > | > base <- A > > Both BBs point to the same BDS now and B is actually closed. Correct. Swapping was just a trick to do things underneath the device's feet, effectively emulating BlockBackends. Since I'm not very much up-to-date with the block device graph stuff, do BDSes have a list of BlockBackends that point to them, so that they can redirect all those BlockBackends to the backing file? Paolo