[Qemu-devel] [PATCH] more check for replaced node

[Qemu-devel] [PATCH] more check for replaced node

[Wen Congyang]

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 7168575..70ee0f6 100644
--- a/block.c
+++ b/block.c
@@ -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;
diff --git a/block/mirror.c b/block/mirror.c
index 0d06cc2..f132f35 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -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;
diff --git a/blockdev.c b/blockdev.c
index f3a3097..a0e13b0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -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);
diff --git a/include/block/block.h b/include/block/block.h
index 07bb724..e0dad54 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -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,

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3362 bytes --]

On Thu, Jun 25, 2015 at 02:55:10PM +0800, Wen Congyang wrote:
> 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(-)

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.

> diff --git a/block.c b/block.c
> index 7168575..70ee0f6 100644
> --- a/block.c
> +++ b/block.c
> @@ -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;
> diff --git a/block/mirror.c b/block/mirror.c
> index 0d06cc2..f132f35 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -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;
> diff --git a/blockdev.c b/blockdev.c
> index f3a3097..a0e13b0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -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);
> diff --git a/include/block/block.h b/include/block/block.h
> index 07bb724..e0dad54 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -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,

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Wen Congyang]

On 07/01/2015 04:39 PM, Stefan Hajnoczi wrote:
> On Thu, Jun 25, 2015 at 02:55:10PM +0800, Wen Congyang wrote:
>> 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(-)
> 
> 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

> 
> Leaving this patch to Jeff since I'm CC only but the email is to Jeff.
> 
>> diff --git a/block.c b/block.c
>> index 7168575..70ee0f6 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -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;
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 0d06cc2..f132f35 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -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;
>> diff --git a/blockdev.c b/blockdev.c
>> index f3a3097..a0e13b0 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -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);
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 07bb724..e0dad54 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -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,

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1188 bytes --]

On Wed, Jul 01, 2015 at 04:49:40PM +0800, Wen Congyang wrote:
> On 07/01/2015 04:39 PM, Stefan Hajnoczi wrote:
> > On Thu, Jun 25, 2015 at 02:55:10PM +0800, Wen Congyang wrote:
> >> 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(-)
> > 
> > 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

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Wen Congyang]

On 07/02/2015 10:48 PM, Stefan Hajnoczi wrote:
> On Wed, Jul 01, 2015 at 04:49:40PM +0800, Wen Congyang wrote:
>> On 07/01/2015 04:39 PM, Stefan Hajnoczi wrote:
>>> On Thu, Jun 25, 2015 at 02:55:10PM +0800, Wen Congyang wrote:
>>>> 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(-)
>>>
>>> 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.

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

> 
> Stefan
> 

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

On Mon, Jul 13, 2015 at 10:41:53AM +0800, Wen Congyang wrote:
> On 07/02/2015 10:48 PM, Stefan Hajnoczi wrote:
> > On Wed, Jul 01, 2015 at 04:49:40PM +0800, Wen Congyang wrote:
> >> On 07/01/2015 04:39 PM, Stefan Hajnoczi wrote:
> >>> On Thu, Jun 25, 2015 at 02:55:10PM +0800, Wen Congyang wrote:
> >>>> 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(-)
> >>>
> >>> 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.
> 
> 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 for explaining.  That makes sense.

Please resend with a commit description.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]