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

[Kevin Wolf <kwolf@redhat.com>]

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.

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.

Kevin