[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 2/2] block: Inactivate all children
From: |
Fam Zheng |
Subject: |
Re: [Qemu-block] [PATCH 2/2] block: Inactivate all children |
Date: |
Tue, 10 May 2016 11:23:02 +0800 |
User-agent: |
Mutt/1.6.1 (2016-04-27) |
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 <address@hidden>
> > >
> > > 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