[Qemu-devel] [PATCH 0/2] block: More complete inactivate/invalidate of on graph

[Qemu-devel] [PATCH 0/2] block: More complete inactivate/invalidate of on graph

[Fam Zheng]

If qcow2 is a quorum child, we currently don't properly invalidate or
inactivate it during migration. Recurse into the whole subtree (subgraph) in
both bdrv_invalidate_cache and bdrv_inactivate.



Fam Zheng (2):
  block: Invalidate all children
  block: Inactivate all children

 block.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

-- 
2.8.0

[Qemu-devel] [PATCH 1/2] block: Invalidate all children

[Fam Zheng]

Currently we only recurse to bs->file, which will miss the children in quorum
and VMDK.

Recurse into the whole subtree to avoid that.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index d4939b4..fa8b38f 100644
--- a/block.c
+++ b/block.c
@@ -3201,6 +3201,7 @@ void bdrv_init_with_whitelist(void)
 
 void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
 {
+    BdrvChild *child;
     Error *local_err = NULL;
     int ret;
 
@@ -3215,13 +3216,20 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
 
     if (bs->drv->bdrv_invalidate_cache) {
         bs->drv->bdrv_invalidate_cache(bs, &local_err);
-    } else if (bs->file) {
-        bdrv_invalidate_cache(bs->file->bs, &local_err);
+        if (local_err) {
+            bs->open_flags |= BDRV_O_INACTIVE;
+            error_propagate(errp, local_err);
+            return;
+        }
     }
-    if (local_err) {
-        bs->open_flags |= BDRV_O_INACTIVE;
-        error_propagate(errp, local_err);
-        return;
+
+    QLIST_FOREACH(child, &bs->children, next) {
+        bdrv_invalidate_cache(child->bs, &local_err);
+        if (local_err) {
+            bs->open_flags |= BDRV_O_INACTIVE;
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     ret = refresh_total_sectors(bs, bs->total_sectors);
-- 
2.8.0

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

[Changlong Xie]

On 04/19/2016 09:42 AM, Fam Zheng wrote:
> Currently we only recurse to bs->file, which will miss the children in quorum
> and VMDK.
>
> Recurse into the whole subtree to avoid that.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block.c | 20 ++++++++++++++------
>   1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/block.c b/block.c
> index d4939b4..fa8b38f 100644
> --- a/block.c
> +++ b/block.c
> @@ -3201,6 +3201,7 @@ void bdrv_init_with_whitelist(void)
>
>   void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
>   {
> +    BdrvChild *child;
>       Error *local_err = NULL;
>       int ret;
>
> @@ -3215,13 +3216,20 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
>
>       if (bs->drv->bdrv_invalidate_cache) {
>           bs->drv->bdrv_invalidate_cache(bs, &local_err);

Kevin's commit 3456a8d1 introduced unexpected two spaces in this 
function, would you like remove it?

-    if (bs->drv && bs->drv->bdrv_invalidate_cache) {
+    if (!bs->drv)  {
+        return;
+    }

> -    } else if (bs->file) {
> -        bdrv_invalidate_cache(bs->file->bs, &local_err);
> +        if (local_err) {
> +            bs->open_flags |= BDRV_O_INACTIVE;
> +            error_propagate(errp, local_err);
> +            return;
> +        }
>       }
> -    if (local_err) {
> -        bs->open_flags |= BDRV_O_INACTIVE;
> -        error_propagate(errp, local_err);
> -        return;
> +
> +    QLIST_FOREACH(child, &bs->children, next) {
> +        bdrv_invalidate_cache(child->bs, &local_err);
> +        if (local_err) {
> +            bs->open_flags |= BDRV_O_INACTIVE;
> +            error_propagate(errp, local_err);
> +            return;

If we really need return here? I just mean for example, Quorum A has 3 
children(children.0, children.1, children.2), if we invalidate 
children.1 failed, then we are not going to invalidate children.2

> +        }
>       }
>
>       ret = refresh_total_sectors(bs, bs->total_sectors);
>

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

[Fam Zheng]

On Tue, 04/19 16:44, Changlong Xie wrote:
> On 04/19/2016 09:42 AM, Fam Zheng wrote:
> >Currently we only recurse to bs->file, which will miss the children in quorum
> >and VMDK.
> >
> >Recurse into the whole subtree to avoid that.
> >
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  block.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> >
> >diff --git a/block.c b/block.c
> >index d4939b4..fa8b38f 100644
> >--- a/block.c
> >+++ b/block.c
> >@@ -3201,6 +3201,7 @@ void bdrv_init_with_whitelist(void)
> >
> >  void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
> >  {
> >+    BdrvChild *child;
> >      Error *local_err = NULL;
> >      int ret;
> >
> >@@ -3215,13 +3216,20 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
> >
> >      if (bs->drv->bdrv_invalidate_cache) {
> >          bs->drv->bdrv_invalidate_cache(bs, &local_err);
> 
> Kevin's commit 3456a8d1 introduced unexpected two spaces in this function,
> would you like remove it?

No, that's what to do in this patch. I'd just leave it.

> 
> -    if (bs->drv && bs->drv->bdrv_invalidate_cache) {
> +    if (!bs->drv)  {
> +        return;
> +    }
> 
> >-    } else if (bs->file) {
> >-        bdrv_invalidate_cache(bs->file->bs, &local_err);
> >+        if (local_err) {
> >+            bs->open_flags |= BDRV_O_INACTIVE;
> >+            error_propagate(errp, local_err);
> >+            return;
> >+        }
> >      }
> >-    if (local_err) {
> >-        bs->open_flags |= BDRV_O_INACTIVE;
> >-        error_propagate(errp, local_err);
> >-        return;
> >+
> >+    QLIST_FOREACH(child, &bs->children, next) {
> >+        bdrv_invalidate_cache(child->bs, &local_err);
> >+        if (local_err) {
> >+            bs->open_flags |= BDRV_O_INACTIVE;
> >+            error_propagate(errp, local_err);
> >+            return;
> 
> If we really need return here? I just mean for example, Quorum A has 3
> children(children.0, children.1, children.2), if we invalidate children.1
> failed, then we are not going to invalidate children.2

We report error anyway and the caller will abort the operation, it's not very
useful to continue.

Fam

> 
> >+        }
> >      }
> >
> >      ret = refresh_total_sectors(bs, bs->total_sectors);
> >
> 
> 
> 

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

[Kevin Wolf]

Am 19.04.2016 um 03:42 hat Fam Zheng geschrieben:
> Currently we only recurse to bs->file, which will miss the children in quorum
> and VMDK.
> 
> Recurse into the whole subtree to avoid that.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index d4939b4..fa8b38f 100644
> --- a/block.c
> +++ b/block.c
> @@ -3201,6 +3201,7 @@ void bdrv_init_with_whitelist(void)
>  
>  void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
>  {
> +    BdrvChild *child;
>      Error *local_err = NULL;
>      int ret;
>  
> @@ -3215,13 +3216,20 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
>  
>      if (bs->drv->bdrv_invalidate_cache) {
>          bs->drv->bdrv_invalidate_cache(bs, &local_err);
> -    } else if (bs->file) {
> -        bdrv_invalidate_cache(bs->file->bs, &local_err);

The old behaviour was that we only recurse for bs->file if the block
driver doesn't have its own implementation.

This means that in qcow2, for example, we call bdrv_invalidate_cache()
explicitly for bs->file. If we can already invalidate it here, the call
inside qcow2 and probably other drivers could go away.

Kevin

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

[Fam Zheng]

On Wed, 05/04 12:10, Kevin Wolf wrote:
> Am 19.04.2016 um 03:42 hat Fam Zheng geschrieben:
> > Currently we only recurse to bs->file, which will miss the children in quorum
> > and VMDK.
> > 
> > Recurse into the whole subtree to avoid that.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index d4939b4..fa8b38f 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -3201,6 +3201,7 @@ void bdrv_init_with_whitelist(void)
> >  
> >  void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
> >  {
> > +    BdrvChild *child;
> >      Error *local_err = NULL;
> >      int ret;
> >  
> > @@ -3215,13 +3216,20 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
> >  
> >      if (bs->drv->bdrv_invalidate_cache) {
> >          bs->drv->bdrv_invalidate_cache(bs, &local_err);
> > -    } else if (bs->file) {
> > -        bdrv_invalidate_cache(bs->file->bs, &local_err);
> 
> The old behaviour was that we only recurse for bs->file if the block
> driver doesn't have its own implementation.
> 
> This means that in qcow2, for example, we call bdrv_invalidate_cache()
> explicitly for bs->file. If we can already invalidate it here, the call
> inside qcow2 and probably other drivers could go away.

Yes, will update. Thanks.

Fam

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

[Fam Zheng]

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(+)

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

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

[Kevin Wolf]

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
> 

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

[Fam Zheng]

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

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

[Kevin Wolf]

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

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

[Fam Zheng]

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

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

[Kevin Wolf]

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

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

[Fam Zheng]

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