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 <f...@redhat.com> > > > > > > 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
