From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35678) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1azyGM-0003R9-EF for qemu-devel@nongnu.org; Mon, 09 May 2016 23:23:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1azyGK-0001vw-2l for qemu-devel@nongnu.org; Mon, 09 May 2016 23:23:09 -0400 Date: Tue, 10 May 2016 11:23:02 +0800 From: Fam Zheng Message-ID: <20160510032302.GC1207@ad.usersys.redhat.com> References: <1461030177-29144-1-git-send-email-famz@redhat.com> <1461030177-29144-3-git-send-email-famz@redhat.com> <20160504101242.GC14972@noname.str.redhat.com> <20160505003224.GA13202@ad.usersys.redhat.com> <20160506074907.GA5093@noname.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160506074907.GA5093@noname.redhat.com> Subject: Re: [Qemu-devel] [PATCH 2/2] block: Inactivate all children List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Max Reitz , qemu-block@nongnu.org On Fri, 05/06 09:49, Kevin Wolf wrote: > Am 05.05.2016 um 02:32 hat Fam Zheng geschrieben: > > On Wed, 05/04 12:12, Kevin Wolf wrote: > > > Am 19.04.2016 um 03:42 hat Fam Zheng geschrieben: > > > > Currently we only inactivate the top BDS. Actually bdrv_inactivate > > > > should be the opposite of bdrv_invalidate_cache. > > > > > > > > Recurse into the whole subtree instead. > > > > > > > > Signed-off-by: Fam Zheng > > > > > > Did you actually test this? > > > > > > I would expect that bs->drv->bdrv_inactivate() fails now (as in > > > assertion failure) if it has anything to flush to the image because > > > bs->file has already be inactivated before. I think children need to be > > > inactived after their parents. > > > > OK, my test apparently failed to trigger that bdrv_pwritv() path. Good catch! > > > > > > > > Nodes with multiple parents could actually become even more > > > interesting... > > > > I'll make it two passes recursion: one for calling drv->bdrv_inactivate and the > > other for setting BDRV_O_INACTIVATE. > > Though that would assume that the .bdrv_inactivate() implementation of > drivers doesn't already bring the BDS into a state where further writes > aren't possible. I'm not sure if that's a good assumption to make, even > though it's currently true for qcow2. > > For example, imagine we went forward with format-based image locking. > The first .bdrv_inactivate() would then already release the lock, we > can't continue writing after that. we only need to make sure all cache of all images is flushed when bdrv_inactivate_all() returns, and similarly, that the cache of one image is flushed when .bdrv_inactivate() returns. The releasing of the lock is an explicit callback and should be place in bdrv_inactivate() right above setting of BDRV_O_INACTIVATE. This is the case in my image locking series. > > Maybe we need something like an "active reference counter", and we > decrement that for all children and only call their .bdrv_inactivate() > when it arrives at 0. That should work, but the effect of the counters are local to one invocation of bdrv_inactivate_all(), and is not really necessary if we do as above. Fam