more check for replaced node
PATCH 1/1 more check for replaced node
--- 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
@@ -4033,7 +4033,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;
@@ -4056,7 +4057,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
@@ -612,7 +612,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
@@ -2756,7 +2756,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
@@ -311,7 +311,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,
Quoted 7 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(-)
This patch is missing a commit description. What is the justification for this change?
Leaving this patch to Jeff since I'm CC only but the email is to Jeff.
Quoted 64 lines
block.c +3
-2@@ -4033,7 +4033,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;@@ -4056,7 +4057,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@@ -612,7 +612,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@@ -2756,7 +2756,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@@ -311,7 +311,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,
Quoted 11 lines · Stefan Hajnoczi
Quoted 7 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(-)This patch is missing a commit description. What is the justification for this change?
Sorry, I forgot to add commit messages..
Without this patch, the replace node can be any node, and it can be top BDS with BB, or another quorum's child. With this patch, the replace node must be this quorum's child.
Thanks
Wen Congyang
Quoted 67 lines
Leaving this patch to Jeff since I'm CC only but the email is to Jeff.
Quoted 64 lines
block.c +3
-2@@ -4033,7 +4033,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;@@ -4056,7 +4057,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@@ -612,7 +612,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@@ -2756,7 +2756,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@@ -311,7 +311,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,
Quoted 10 lines · Wen Congyang
Quoted 9 lines · Stefan Hajnoczi
Quoted 7 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(-)
Quoted 8 lines
Quoted 2 lines
This patch is missing a commit description. What is the justification for this change?
Sorry, I forgot to add commit messages..
Without this patch, the replace node can be any node, and it can be top BDS with BB, or another quorum's child. With this patch, the replace node must be this quorum's child.
I think the point of the replace operation was to swap a quorum child with a new drive. It sounds like this patch will break that use case?
The idea was that a failed child needs to be replaced. The user adds a new -drive and then uses the mirror+replace to include it into the quorum. I think the new child is not be part of the quorum BDS graph until replacement occurs.
Stefan
Quoted 20 lines · Stefan Hajnoczi
Quoted 10 lines · Wen Congyang
Quoted 9 lines · Stefan Hajnoczi
Quoted 7 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(-)Quoted 8 lines
Quoted 2 lines
This patch is missing a commit description. What is the justification for this change?
Sorry, I forgot to add commit messages..
Without this patch, the replace node can be any node, and it can be top BDS with BB, or another quorum's child. With this patch, the replace node must be this quorum's child.
Quoted 7 lines
I think the point of the replace operation was to swap a quorum child with a new drive. It sounds like this patch will break that use case?
The idea was that a failed child needs to be replaced. The user adds a new -drive and then uses the mirror+replace to include it into the quorum. I think the new child is not be part of the quorum BDS graph until replacement occurs.
bs/s->common.bs is quorum, and to_replace is the broken child. The new child is target_bs. With this patch, we just check if the broken child is part of the quorum BDS.
Do I misunderstand something?
Thanks
Wen Congyang
Quoted 3 lines
Stefan
Quoted 29 lines · Wen Congyang
Quoted 20 lines · Stefan Hajnoczi
Quoted 10 lines · Wen Congyang
Quoted 9 lines · Stefan Hajnoczi
Quoted 7 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(-)Quoted 8 lines
Quoted 2 lines
This patch is missing a commit description. What is the justification for this change?
Sorry, I forgot to add commit messages..
Without this patch, the replace node can be any node, and it can be top BDS with BB, or another quorum's child. With this patch, the replace node must be this quorum's child.
Quoted 7 lines
I think the point of the replace operation was to swap a quorum child with a new drive. It sounds like this patch will break that use case?
The idea was that a failed child needs to be replaced. The user adds a new -drive and then uses the mirror+replace to include it into the quorum. I think the new child is not be part of the quorum BDS graph until replacement occurs.
Quoted 5 lines
bs/s->common.bs is quorum, and
to_replaceis the broken child. The new child istarget_bs. With this patch, we just check if the broken child is part of the quorum BDS.Do I misunderstand something?
Thanks for explaining. That makes sense.
Please resend with a commit description.
Stefan