From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57825) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFDEH-0005uo-NO for qemu-devel@nongnu.org; Tue, 14 Jul 2015 23:19:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZFDED-0000mr-Ll for qemu-devel@nongnu.org; Tue, 14 Jul 2015 23:19:29 -0400 Received: from [59.151.112.132] (port=3385 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFDED-0000Zm-1M for qemu-devel@nongnu.org; Tue, 14 Jul 2015 23:19:25 -0400 Message-ID: <55A5D20C.8010605@cn.fujitsu.com> Date: Wed, 15 Jul 2015 11:22:52 +0800 From: Wen Congyang MIME-Version: 1.0 References: <55A5BA2D.4060402@cn.fujitsu.com> <20150715030532.GB2412@ad.nay.redhat.com> In-Reply-To: <20150715030532.GB2412@ad.nay.redhat.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] more check for replaced node List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: benoit.canet@irqsave.net, Jeff Cody , qemu-devl , Stefan Hajnoczi 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 >> --- >> 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 don't know why we check it twice. And I think it is redundant too. 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 >> > . >