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

[Kevin Wolf <kwolf@redhat.com>]

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 <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.

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