[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] more check for replaced node
From: |
Wen Congyang |
Subject: |
Re: [Qemu-devel] [PATCH v2] more check for replaced node |
Date: |
Fri, 17 Jul 2015 09:37:52 +0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 |
On 07/15/2015 11:05 AM, Fam Zheng wrote:
> On Wed, 07/15 09:41, 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 <address@hidden>
>> ---
>> 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 238a070..b81077e 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -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?
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
>
> Fam
>
>> if (!s->to_replace) {
>> error_propagate(errp, local_err);
>> return;
>> diff --git a/blockdev.c b/blockdev.c
>> index c11611d..bf12e2e 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -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);
>> 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
>>
> .
>