Re: [Qemu-devel] [PATCH v2] more check for replaced node

[Wen Congyang <wency@cn.fujitsu.com>]

On 07/16/2015 09:36 PM, Stefan Hajnoczi wrote:
> On Wed, Jul 15, 2015 at 11:22:52AM +0800, Wen Congyang wrote:
>> 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 <wency@cn.fujitsu.com>
>>>> ---
>>>>  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.
> 
> 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