Re: [Qemu-block] [PATCH v12 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Alberto Garcia]

On Mon 14 Mar 2016 07:02:08 AM CET, Changlong Xie <address@hidden> wrote:

>>> @@ -81,6 +82,8 @@ typedef struct BDRVQuorumState {
>>>       bool rewrite_corrupted;/* true if the driver must rewrite-on-read 
>>> corrupted
>>>                               * block if Quorum is reached.
>>>                               */
>>> +    unsigned long *index_bitmap;
>
> Hi Berto
>
> *NOTE*, In the old version, we just used "bs->node_name", but in the
> lastest one, as Kevin suggested we introduce
> "child->child_name"(formart as "children.xxx"), this is the key cause
> why we need this two functions here.

I'm sorry I missed this discussion earlier. Your code seems technically
correct but I have several questions:

- I read that one of the reasons for this change is that "In theory, the
  same node could be attached twice to the same parent in different
  roles.". Is there any example of that? What's the use case?

- How do you obtain the child name?

- I see that if you have children.0 and children.1 (let's say hd0.qcow2
  and hd1.qcow2), then you remove children.0 and add it again, it will
  keep the 'children.0' name (that's what the bitmap is for if I'm
  understanding it correctly). However the position in the s->children
  array will change because you do memmove() when you remove children.0
  and then add it again to the end of the array.

  Initial status:

    s->children[0] <--> "children.0" (hd0.qcow2)
    s->children[1] <--> "children.1" (hd1.qcow2)

  children.0 (hd0.qcow2) is removed:

    s->children[0] <--> "children.1" (hd1.qcow2)

  children.0 (hd0.qcow2) is added again:

    s->children[0] <--> "children.1" (hd1.qcow2)
    s->children[1] <--> "children.0" (hd0.qcow2)

  Is this correct? Is this the indented behavior? Since you are reading
  in FIFO mode, now hd1.qcow2 will always be read first, so if
  children.1 was the secondary disk, it has just become the primary.

I also think that it would be great to have tests for this
functionality, but they can be added later.

Thanks,

Berto