Am 10.05.2016 um 05:23 hat Fam Zheng geschrieben: > 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.
Fair enough. My series didn't have a separate callback, but with yours that should be working. So is the semantics of .bdrv_inactivate() basically "bdrv_flush, and I really mean it"? > > 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. Agreed. Kevin
