[Qemu-devel] [PATCH v3] more check for replaced node

[Qemu-devel] [PATCH v3] more check for replaced node

[Wen Congyang]

We use mirror+replace to fix quorum's broken child. bs/s->common.bs
is quorum, and to_replace is the broken child. The new child is target_bs.
Without this patch, the replace node can be any node, and it can be
top BDS with BB, or another quorum's child. We just check if the broken
child is part of the quorum BDS in this patch.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 block.c               | 5 +++--
 block/mirror.c        | 3 ++-
 blockdev.c            | 2 +-
 include/block/block.h | 3 ++-
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index d088ee0..090923c 100644
--- a/block.c
+++ b/block.c
@@ -4077,7 +4077,8 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
     return false;
 }
 
-BlockDriverState *check_to_replace_node(const char *node_name, Error **errp)
+BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
+                                        const char *node_name, Error **errp)
 {
     BlockDriverState *to_replace_bs = bdrv_find_node(node_name);
     AioContext *aio_context;
@@ -4100,7 +4101,7 @@ BlockDriverState *check_to_replace_node(const char *node_name, Error **errp)
      * Another benefit is that this tests exclude backing files which are
      * blocked by the backing blockers.
      */
-    if (!bdrv_is_first_non_filter(to_replace_bs)) {
+    if (!bdrv_recurse_is_first_non_filter(parent_bs, to_replace_bs)) {
         error_setg(errp, "Only top most non filter can be replaced");
         to_replace_bs = NULL;
         goto out;
diff --git a/block/mirror.c b/block/mirror.c
index 323f747..68067fa 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -637,9 +637,9 @@ static void mirror_complete(BlockJob *job, Error **errp)
     if (s->replaces) {
         AioContext *replace_aio_context;
 
-        s->to_replace = check_to_replace_node(s->replaces, &local_err);
+        s->to_replace = bdrv_find_node(s->replaces);
         if (!s->to_replace) {
-            error_propagate(errp, local_err);
+            error_setg(errp, "Node name '%s' not found", s->replaces);
             return;
         }
 
diff --git a/blockdev.c b/blockdev.c
index 62a4586..07c72d3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2754,7 +2754,7 @@ void qmp_drive_mirror(const char *device, const char *target,
             goto out;
         }
 
-        to_replace_bs = check_to_replace_node(replaces, &local_err);
+        to_replace_bs = check_to_replace_node(bs, replaces, &local_err);
 
         if (!to_replace_bs) {
             error_propagate(errp, local_err);
diff --git a/include/block/block.h b/include/block/block.h
index 37916f7..608cd4e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -317,7 +317,8 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
 bool bdrv_is_first_non_filter(BlockDriverState *candidate);
 
 /* check if a named node can be replaced when doing drive-mirror */
-BlockDriverState *check_to_replace_node(const char *node_name, Error **errp);
+BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
+                                        const char *node_name, Error **errp);
 
 /* async block I/O */
 typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
-- 
2.4.3

Re: [Qemu-devel] [PATCH v3] more check for replaced node

[Wen Congyang]

Ping...

On 07/17/2015 10:12 AM, Wen Congyang wrote:
> We use mirror+replace to fix quorum's broken child. bs/s->common.bs
> is quorum, and to_replace is the broken child. The new child is target_bs.
> Without this patch, the replace node can be any node, and it can be
> top BDS with BB, or another quorum's child. We just check if the broken
> child is part of the quorum BDS in this patch.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  block.c               | 5 +++--
>  block/mirror.c        | 3 ++-
>  blockdev.c            | 2 +-
>  include/block/block.h | 3 ++-
>  4 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index d088ee0..090923c 100644
> --- a/block.c
> +++ b/block.c
> @@ -4077,7 +4077,8 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
>      return false;
>  }
>  
> -BlockDriverState *check_to_replace_node(const char *node_name, Error **errp)
> +BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
> +                                        const char *node_name, Error **errp)
>  {
>      BlockDriverState *to_replace_bs = bdrv_find_node(node_name);
>      AioContext *aio_context;
> @@ -4100,7 +4101,7 @@ BlockDriverState *check_to_replace_node(const char *node_name, Error **errp)
>       * Another benefit is that this tests exclude backing files which are
>       * blocked by the backing blockers.
>       */
> -    if (!bdrv_is_first_non_filter(to_replace_bs)) {
> +    if (!bdrv_recurse_is_first_non_filter(parent_bs, to_replace_bs)) {
>          error_setg(errp, "Only top most non filter can be replaced");
>          to_replace_bs = NULL;
>          goto out;
> diff --git a/block/mirror.c b/block/mirror.c
> index 323f747..68067fa 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -637,9 +637,9 @@ static void mirror_complete(BlockJob *job, Error **errp)
>      if (s->replaces) {
>          AioContext *replace_aio_context;
>  
> -        s->to_replace = check_to_replace_node(s->replaces, &local_err);
> +        s->to_replace = bdrv_find_node(s->replaces);
>          if (!s->to_replace) {
> -            error_propagate(errp, local_err);
> +            error_setg(errp, "Node name '%s' not found", s->replaces);
>              return;
>          }
>  
> diff --git a/blockdev.c b/blockdev.c
> index 62a4586..07c72d3 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2754,7 +2754,7 @@ void qmp_drive_mirror(const char *device, const char *target,
>              goto out;
>          }
>  
> -        to_replace_bs = check_to_replace_node(replaces, &local_err);
> +        to_replace_bs = check_to_replace_node(bs, replaces, &local_err);
>  
>          if (!to_replace_bs) {
>              error_propagate(errp, local_err);
> diff --git a/include/block/block.h b/include/block/block.h
> index 37916f7..608cd4e 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -317,7 +317,8 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
>  bool bdrv_is_first_non_filter(BlockDriverState *candidate);
>  
>  /* check if a named node can be replaced when doing drive-mirror */
> -BlockDriverState *check_to_replace_node(const char *node_name, Error **errp);
> +BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
> +                                        const char *node_name, Error **errp);
>  
>  /* async block I/O */
>  typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
> 

Re: [Qemu-devel] [PATCH v3] more check for replaced node

[Stefan Hajnoczi]

On Fri, Jul 17, 2015 at 10:12:22AM +0800, Wen Congyang wrote:
> We use mirror+replace to fix quorum's broken child. bs/s->common.bs
> is quorum, and to_replace is the broken child. The new child is target_bs.
> Without this patch, the replace node can be any node, and it can be
> top BDS with BB, or another quorum's child. We just check if the broken
> child is part of the quorum BDS in this patch.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  block.c               | 5 +++--
>  block/mirror.c        | 3 ++-
>  blockdev.c            | 2 +-
>  include/block/block.h | 3 ++-
>  4 files changed, 8 insertions(+), 5 deletions(-)

There has been no further discussion.  I think the patch is a good idea,
so I'm applying it.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan