| Message ID | 1461030177-29144-3-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. Nodes with multiple parents could actually become even more interesting... Kevin > block.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/block.c b/block.c > index fa8b38f..9a84ed1 100644 > --- a/block.c > +++ b/block.c > @@ -3260,8 +3260,16 @@ void bdrv_invalidate_cache_all(Error **errp) > > static int bdrv_inactivate(BlockDriverState *bs) > { > + BdrvChild *child; > int ret; > > + QLIST_FOREACH(child, &bs->children, next) { > + ret = bdrv_inactivate(child->bs); > + if (ret < 0) { > + return ret; > + } > + } > + > if (bs->drv->bdrv_inactivate) { > ret = bs->drv->bdrv_inactivate(bs); > if (ret < 0) { > -- > 2.8.0 >
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. Fam
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
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
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
On Tue, 05/10 10:33, Kevin Wolf wrote: > > 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"? Yes. > > > > 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. Working on another version now. Fam
diff --git a/block.c b/block.c index fa8b38f..9a84ed1 100644 --- a/block.c +++ b/block.c @@ -3260,8 +3260,16 @@ void bdrv_invalidate_cache_all(Error **errp) static int bdrv_inactivate(BlockDriverState *bs) { + BdrvChild *child; int ret; + QLIST_FOREACH(child, &bs->children, next) { + ret = bdrv_inactivate(child->bs); + if (ret < 0) { + return ret; + } + } + if (bs->drv->bdrv_inactivate) { ret = bs->drv->bdrv_inactivate(bs); if (ret < 0) {
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> --- block.c | 8 ++++++++ 1 file changed, 8 insertions(+)