more check for replaced node

PATCH Superseded Wen Congyang 3 participants 6 messages Jul 15, 2015 – Jul 17, 2015 Block layer

Share & tools

Wen Congyang

author

2015-07-15 01:37 UTC #0 lore raw

PATCH v2 1/1 more check for replaced node

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 <[email protected]>

---
 block.c               | 5 +++--
 block/mirror.c        | 3 ++-
 blockdev.c            | 2 +-
 include/block/block.h | 3 ++-
 4 files changed, 8 insertions(+), 5 deletions(-)

block.c +3 -2

index d088ee0..090923c 100644
@@ -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;

block/mirror.c +2 -1

index 238a070..b81077e 100644
@@ -626,7 +626,8 @@ 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 = check_to_replace_node(s->common.bs, s->replaces,
+                                              &local_err);
         if (!s->to_replace) {
             error_propagate(errp, local_err);
             return;

blockdev.c +1 -1

index c11611d..bf12e2e 100644
@@ -2757,7 +2757,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);

include/block/block.h +2 -1

index 37916f7..608cd4e 100644
@@ -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,

Signature · 1 line

2.4.3

Fam Zheng

2015-07-15 03:05 UTC #1 lore raw

Quoted 42 lines · Wen Congyang

Signed-off-by Wen Congyang <[email protected]>

---
 block.c               | 5 +++--
 block/mirror.c        | 3 ++-
 blockdev.c            | 2 +-
 include/block/block.h | 3 ++-
 4 files changed, 8 insertions(+), 5 deletions(-)

block.c +3 -2

index d088ee0..090923c 100644
@@ -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;

block/mirror.c +0 -0

index 238a070..b81077e 100644

block/mirror.c static void mirror_complete(BlockJob *job, Error **errp) · line 630

Quoted 6 lines

block/mirror.c +2 -1

@@ -626,7 +626,8 @@ 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 = check_to_replace_node(s->common.bs, s->replaces,
+                                              &local_err);

Why is the check in qmp_drive_mirror not enough? Isn't this redundant?

Fam

Quoted 33 lines

block/mirror.c +0 -0

         if (!s->to_replace) {
             error_propagate(errp, local_err);
             return;

blockdev.c +1 -1

index c11611d..bf12e2e 100644
@@ -2757,7 +2757,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);

include/block/block.h +2 -1

index 37916f7..608cd4e 100644
@@ -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,

Signature · 1 line

2.4.3

Wen Congyang

author

2015-07-15 03:19 UTC #2 lore raw

Quoted 46 lines · Fam Zheng

Quoted 45 lines · Wen Congyang

Signed-off-by Wen Congyang <[email protected]>

---
 block.c               | 5 +++--
 block/mirror.c        | 3 ++-
 blockdev.c            | 2 +-
 include/block/block.h | 3 ++-
 4 files changed, 8 insertions(+), 5 deletions(-)

block.c +3 -2

index d088ee0..090923c 100644
@@ -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;

block/mirror.c +0 -0

index 238a070..b81077e 100644
@@ -626,7 +626,8 @@ static void mirror_complete(BlockJob *job, Error **errp)
     if (s->replaces) {
         AioContext *replace_aio_context;

Quoted 5 lines

block/mirror.c static void mirror_complete(BlockJob *job, Error **errp) · line 630
Quoted 3 lines
block/mirror.c +2 -1
-        s->to_replace = check_to_replace_node(s->replaces, &local_err);
+        s->to_replace = check_to_replace_node(s->common.bs, s->replaces,
+                                              &local_err);
Why is the check in qmp_drive_mirror not enough? Isn't this redundant?

I don't know why we check it twice. And I think it is redundant too.

Thanks
Wen Congyang

Quoted 38 lines

Fam

Quoted 33 lines

block/mirror.c +0 -0

         if (!s->to_replace) {
             error_propagate(errp, local_err);
             return;

blockdev.c +1 -1

index c11611d..bf12e2e 100644
@@ -2757,7 +2757,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);

include/block/block.h +2 -1

index 37916f7..608cd4e 100644
@@ -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,

Signature · 1 line

2.4.3

Stefan Hajnoczi

2015-07-16 13:36 UTC #3 lore raw

Quoted 47 lines · Wen Congyang

Quoted 46 lines · Fam Zheng

Quoted 45 lines · Wen Congyang

Signed-off-by Wen Congyang <[email protected]>

---
 block.c               | 5 +++--
 block/mirror.c        | 3 ++-
 blockdev.c            | 2 +-
 include/block/block.h | 3 ++-
 4 files changed, 8 insertions(+), 5 deletions(-)

block.c +3 -2

index d088ee0..090923c 100644
@@ -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;

block/mirror.c +0 -0

index 238a070..b81077e 100644
@@ -626,7 +626,8 @@ static void mirror_complete(BlockJob *job, Error **errp)
     if (s->replaces) {
         AioContext *replace_aio_context;

Quoted 7 lines

Quoted 5 lines
block/mirror.c static void mirror_complete(BlockJob *job, Error **errp) · line 630
Quoted 3 lines
block/mirror.c +2 -1
-        s->to_replace = check_to_replace_node(s->replaces, &local_err);
+        s->to_replace = check_to_replace_node(s->common.bs, s->replaces,
+                                              &local_err);
Why is the check in qmp_drive_mirror not enough? Isn't this redundant?
I don't know why we check it twice. And I think it is redundant too.

Okay, does that mean you are dropping this patch now?

Wen Congyang

author

2015-07-17 01:32 UTC #4 lore raw

Quoted 54 lines · Stefan Hajnoczi

Quoted 53 lines · Wen Congyang

Quoted 51 lines · Fam Zheng

Quoted 42 lines · Wen Congyang

Signed-off-by Wen Congyang <[email protected]>

---
 block.c               | 5 +++--
 block/mirror.c        | 3 ++-
 blockdev.c            | 2 +-
 include/block/block.h | 3 ++-
 4 files changed, 8 insertions(+), 5 deletions(-)

block.c +3 -2

index d088ee0..090923c 100644
@@ -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;

block/mirror.c +0 -0

index 238a070..b81077e 100644

block/mirror.c static void mirror_complete(BlockJob *job, Error **errp) · line 630

Quoted 6 lines

block/mirror.c +2 -1

@@ -626,7 +626,8 @@ 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 = check_to_replace_node(s->common.bs, s->replaces,
+                                              &local_err);

Why is the check in qmp_drive_mirror not enough? Isn't this redundant?

Quoted 4 lines

Quoted 1 line
I don't know why we check it twice. And I think it is redundant too.
Okay, does that mean you are dropping this patch now?

No, we call check_to_replace_node() twice. So we can remove it from mirror_complete().

Will update it in the next version.

Thanks
Wen Congyang

Wen Congyang

author

2015-07-17 01:34 UTC #5 lore raw

In reply to Fam Zheng

Quoted 46 lines · Fam Zheng

Quoted 45 lines · Wen Congyang

Signed-off-by Wen Congyang <[email protected]>

---
 block.c               | 5 +++--
 block/mirror.c        | 3 ++-
 blockdev.c            | 2 +-
 include/block/block.h | 3 ++-
 4 files changed, 8 insertions(+), 5 deletions(-)

block.c +3 -2

index d088ee0..090923c 100644
@@ -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;

block/mirror.c +0 -0

index 238a070..b81077e 100644
@@ -626,7 +626,8 @@ static void mirror_complete(BlockJob *job, Error **errp)
     if (s->replaces) {
         AioContext *replace_aio_context;

Quoted 5 lines

block/mirror.c static void mirror_complete(BlockJob *job, Error **errp) · line 630
Quoted 3 lines
block/mirror.c +2 -1
-        s->to_replace = check_to_replace_node(s->replaces, &local_err);
+        s->to_replace = check_to_replace_node(s->common.bs, s->replaces,
+                                              &local_err);
Why is the check in qmp_drive_mirror not enough? Isn't this redundant?

I guess the reason is that: we only pass replaced node name to the job, and we only need to get the BDS here. So we can use bdrv_find_node() to replace check_to_replace_node() here.

Thanks
Wen Congyang

Quoted 38 lines

Fam

Quoted 33 lines

block/mirror.c +0 -0

         if (!s->to_replace) {
             error_propagate(errp, local_err);
             return;

blockdev.c +1 -1

index c11611d..bf12e2e 100644
@@ -2757,7 +2757,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);

include/block/block.h +2 -1

index 37916f7..608cd4e 100644
@@ -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,

Signature · 1 line

2.4.3