[Qemu-devel] [PATCH v2 0/3] block: More complete inactivate/invalidate of graph nodes

[Qemu-devel] [PATCH v2 0/3] block: More complete inactivate/invalidate of graph nodes

[Fam Zheng]

v2: Two passes inactivation. [Kevin]

For now we only consider bs->file chain. In fact this is incomplete. For
example, if qcow2 is a quorum child, we don't properly invalidate or inactivate
it.  This series recurses into the subtrees in both bdrv_invalidate_cache_all
and bdrv_inactivate_all. This is also necessary to make the image locking
cooperate with migration.


Fam Zheng (3):
  block: Invalidate all children
  block: Drop superfluous invalidating bs->file from drivers
  block: Inactivate all children

 block.c        | 67 +++++++++++++++++++++++++++++++++++++++++++---------------
 block/qcow2.c  |  7 ------
 block/qed.c    |  6 ------
 block/quorum.c | 16 --------------
 4 files changed, 50 insertions(+), 46 deletions(-)

-- 
2.8.2

[Qemu-devel] [PATCH v2 1/3] 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.2

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

[Alberto Garcia]

On Wed 11 May 2016 04:45:33 AM CEST, 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>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH v2 2/3] block: Drop superfluous invalidating bs->file from drivers

[Fam Zheng]

Now they are invalidated by the block layer, so it's not necessary to
do this in block drivers' implementations of .bdrv_invalidate_cache.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/qcow2.c  |  7 -------
 block/qed.c    |  6 ------
 block/quorum.c | 16 ----------------
 3 files changed, 29 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 470734b..d4431e0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1757,13 +1757,6 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
 
     qcow2_close(bs);
 
-    bdrv_invalidate_cache(bs->file->bs, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        bs->drv = NULL;
-        return;
-    }
-
     memset(s, 0, sizeof(BDRVQcow2State));
     options = qdict_clone_shallow(bs->options);
 
diff --git a/block/qed.c b/block/qed.c
index 0af5274..9081941 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1594,12 +1594,6 @@ static void bdrv_qed_invalidate_cache(BlockDriverState *bs, Error **errp)
 
     bdrv_qed_close(bs);
 
-    bdrv_invalidate_cache(bs->file->bs, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
     memset(s, 0, sizeof(BDRVQEDState));
     ret = bdrv_qed_open(bs, NULL, bs->open_flags, &local_err);
     if (local_err) {
diff --git a/block/quorum.c b/block/quorum.c
index da15465..8f7c099 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -747,21 +747,6 @@ static int64_t quorum_getlength(BlockDriverState *bs)
     return result;
 }
 
-static void quorum_invalidate_cache(BlockDriverState *bs, Error **errp)
-{
-    BDRVQuorumState *s = bs->opaque;
-    Error *local_err = NULL;
-    int i;
-
-    for (i = 0; i < s->num_children; i++) {
-        bdrv_invalidate_cache(s->children[i]->bs, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
-        }
-    }
-}
-
 static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
 {
     BDRVQuorumState *s = bs->opaque;
@@ -1070,7 +1055,6 @@ static BlockDriver bdrv_quorum = {
 
     .bdrv_aio_readv                     = quorum_aio_readv,
     .bdrv_aio_writev                    = quorum_aio_writev,
-    .bdrv_invalidate_cache              = quorum_invalidate_cache,
 
     .bdrv_detach_aio_context            = quorum_detach_aio_context,
     .bdrv_attach_aio_context            = quorum_attach_aio_context,
-- 
2.8.2

Re: [Qemu-devel] [PATCH v2 2/3] block: Drop superfluous invalidating bs->file from drivers

[Alberto Garcia]

On Wed 11 May 2016 04:45:34 AM CEST, Fam Zheng wrote:
> Now they are invalidated by the block layer, so it's not necessary to
> do this in block drivers' implementations of .bdrv_invalidate_cache.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH v2 3/3] 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.

Because a node may have multiple parents, and because once
BDRV_O_INACTIVE is set for a node, further writes are not allowed, we
cannot interleave flag settings and .bdrv_inactivate calls (that may
submit write to other nodes in a graph) within a single pass. Therefore
two passes are used here.

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

diff --git a/block.c b/block.c
index fa8b38f..93481c4 100644
--- a/block.c
+++ b/block.c
@@ -3258,38 +3258,63 @@ void bdrv_invalidate_cache_all(Error **errp)
     }
 }
 
-static int bdrv_inactivate(BlockDriverState *bs)
+static int bdrv_inactivate_recurse(BlockDriverState *bs,
+                                   bool setting_flag)
 {
+    BdrvChild *child;
     int ret;
 
-    if (bs->drv->bdrv_inactivate) {
+    if (!setting_flag && bs->drv->bdrv_inactivate) {
         ret = bs->drv->bdrv_inactivate(bs);
         if (ret < 0) {
             return ret;
         }
     }
 
-    bs->open_flags |= BDRV_O_INACTIVE;
+    QLIST_FOREACH(child, &bs->children, next) {
+        ret = bdrv_inactivate_recurse(child->bs, setting_flag);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
+    if (setting_flag) {
+        bs->open_flags |= BDRV_O_INACTIVE;
+    }
     return 0;
 }
 
 int bdrv_inactivate_all(void)
 {
     BlockDriverState *bs = NULL;
-    int ret;
+    int ret = 0;
+    int pass;
 
     while ((bs = bdrv_next(bs)) != NULL) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
+        aio_context_acquire(bdrv_get_aio_context(bs));
+    }
 
-        aio_context_acquire(aio_context);
-        ret = bdrv_inactivate(bs);
-        aio_context_release(aio_context);
-        if (ret < 0) {
-            return ret;
+    /* We do two passes of inactivation. The first pass calls to drivers'
+     * .bdrv_inactivate callbacks recursively so all cache is flushed to disk;
+     * the second pass sets the BDRV_O_INACTIVE flag so that no further write
+     * is allowed. */
+    for (pass = 0; pass < 2; pass++) {
+        bs = NULL;
+        while ((bs = bdrv_next(bs)) != NULL) {
+            ret = bdrv_inactivate_recurse(bs, pass);
+            if (ret < 0) {
+                goto out;
+            }
         }
     }
 
-    return 0;
+out:
+    bs = NULL;
+    while ((bs = bdrv_next(bs)) != NULL) {
+        aio_context_release(bdrv_get_aio_context(bs));
+    }
+
+    return ret;
 }
 
 /**************************************************************/
-- 
2.8.2

Re: [Qemu-devel] [PATCH v2 0/3] block: More complete inactivate/invalidate of graph nodes

[Kevin Wolf]

Am 11.05.2016 um 04:45 hat Fam Zheng geschrieben:
> v2: Two passes inactivation. [Kevin]
> 
> For now we only consider bs->file chain. In fact this is incomplete. For
> example, if qcow2 is a quorum child, we don't properly invalidate or inactivate
> it.  This series recurses into the subtrees in both bdrv_invalidate_cache_all
> and bdrv_inactivate_all. This is also necessary to make the image locking
> cooperate with migration.

Thanks, applied to block-next.

Kevin