Re: [Qemu-devel] [PATCH 2/2] block: Inactivate all children

[Fam Zheng <famz@redhat.com>]

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 <famz@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