From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38518) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFuXe-0005sM-CM for qemu-devel@nongnu.org; Thu, 16 Jul 2015 21:34:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZFuXZ-0003Mb-RJ for qemu-devel@nongnu.org; Thu, 16 Jul 2015 21:34:22 -0400 Received: from [59.151.112.132] (port=45968 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFuXZ-0003F2-1y for qemu-devel@nongnu.org; Thu, 16 Jul 2015 21:34:17 -0400 Message-ID: <55A85C70.6050706@cn.fujitsu.com> Date: Fri, 17 Jul 2015 09:37: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: 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 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 >> > . >