Re: [Qemu-devel] [PATCH v5 2/4] quorum: implement bdrv_add_child() and bdrv_del_child()

[Wen Congyang]

On 10/07/2015 10:12 PM, Alberto Garcia wrote:
> On Tue 22 Sep 2015 09:44:20 AM CEST, Wen Congyang wrote:
> 
>> +++ b/block/quorum.c
>> @@ -66,6 +66,9 @@ typedef struct QuorumVotes {
>>  typedef struct BDRVQuorumState {
>>  BlockDriverState **bs; /* children BlockDriverStates */
>>  int num_children;  /* children count */
>> +int max_children;  /* The maximum children count, we need to 
>> reallocate
>> +* bs if num_children grows larger than maximum.
>> +*/
>>  int threshold; /* if less than threshold children reads gave the
>>  * same result a quorum error occurs.
>>  */
> 
> As you announce in the cover letter of this series, your code depends on
> the parents list patch written by Kevin here:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg04579.html
> 
> As you might be aware, and as part of the same series by Kevin,
> BDRVQuorumState will no longer hold a list of BlockDriverState but a
> list of BdrvChild instead:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg04571.html

I notice that, and I only one patch from Kevin now. I will fix it in the
next version.

> 
>> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState 
>> *child_bs,
>> + Error **errp)
>> +{
>> +BDRVQuorumState *s = bs->opaque;
>> +
>> +bdrv_drain(bs);
>> +
>> +if (s->num_children == s->max_children) {
>> +if (s->max_children >= INT_MAX) {
>> +error_setg(errp, "Too many children");
>> +return;
>> +}
> 
> max_children can never be greater than INT_MAX. Use == instead.
> 
>> +s->bs = g_renew(BlockDriverState *, s->bs, s->max_children + 1);
>> +s->bs[s->num_children] = NULL;
> 
> No need to set the pointer to NULL here, and you are anyway setting the
> pointer to the new child a few lines afterwards.

Yes, I will remove it in the next version.

> 
>> +s->max_children++;
>> +}
>> +
>> +bdrv_ref(child_bs);
>> +bdrv_attach_child(bs, child_bs, _format);
>> +s->bs[s->num_children++] = child_bs;
>> +}
>> +
>> +static void quorum_del_child(BlockDriverState *bs, BlockDriverState 
>> *child_bs,
>> + Error **errp)
>> +{
>> +BDRVQuorumState *s = bs->opaque;
>> +BdrvChild *child;
>> +int i;
>> +
>> +for (i = 0; i < s->num_children; i++) {
>> +if (s->bs[i] == child_bs) {
>> +break;
>> +}
>> +}
>> +
>> +QLIST_FOREACH(child, >children, next) {
>> +if (child->bs == child_bs) {
>> +break;
>> +}
>> +}
>> +
>> +/* we have checked it in bdrv_del_child() */
>> +assert(i < s->num_children && child);
>> +
>> +if (s->num_children <= s->threshold) {
>> +error_setg(errp,
>> +"The number of children cannot be lower than the vote threshold 
>> %d",
>> +s->threshold);
>> +return;
>> +}
>> +
>> +bdrv_drain(bs);
>> +/* We can safely remove this child now */
>> +memmove(>bs[i], >bs[i + 1],
>> +(s->num_children - i - 1) * sizeof(void *));
>> +s->num_children--;
>> +s->bs[s->num_children] = NULL;
> 
> Same here, no one will check or use s->bs[s->num_children] so there's no
> need to make it NULL.
> 
> Apart from the issue of using only part of Kevin's series, the rest are
> minor things.

I will fix it in the next version.

> 
> Thanks and sorry for the late review!

Thanks for your review

Wen Congyang

> 
> Berto
> .
> 

Re: [Qemu-devel] [PATCH v5 2/4] quorum: implement bdrv_add_child() and bdrv_del_child()

[Alberto Garcia]

On Tue 22 Sep 2015 09:44:20 AM CEST, Wen Congyang wrote:

> +++ b/block/quorum.c
> @@ -66,6 +66,9 @@ typedef struct QuorumVotes {
>  typedef struct BDRVQuorumState {
>  BlockDriverState **bs; /* children BlockDriverStates */
>  int num_children;  /* children count */
> +int max_children;  /* The maximum children count, we need to 
> reallocate
> +* bs if num_children grows larger than maximum.
> +*/
>  int threshold; /* if less than threshold children reads gave the
>  * same result a quorum error occurs.
>  */

As you announce in the cover letter of this series, your code depends on
the parents list patch written by Kevin here:

http://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg04579.html

As you might be aware, and as part of the same series by Kevin,
BDRVQuorumState will no longer hold a list of BlockDriverState but a
list of BdrvChild instead:

https://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg04571.html

> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState 
> *child_bs,
> + Error **errp)
> +{
> +BDRVQuorumState *s = bs->opaque;
> +
> +bdrv_drain(bs);
> +
> +if (s->num_children == s->max_children) {
> +if (s->max_children >= INT_MAX) {
> +error_setg(errp, "Too many children");
> +return;
> +}

max_children can never be greater than INT_MAX. Use == instead.

> +s->bs = g_renew(BlockDriverState *, s->bs, s->max_children + 1);
> +s->bs[s->num_children] = NULL;

No need to set the pointer to NULL here, and you are anyway setting the
pointer to the new child a few lines afterwards.

> +s->max_children++;
> +}
> +
> +bdrv_ref(child_bs);
> +bdrv_attach_child(bs, child_bs, _format);
> +s->bs[s->num_children++] = child_bs;
> +}
> +
> +static void quorum_del_child(BlockDriverState *bs, BlockDriverState 
> *child_bs,
> + Error **errp)
> +{
> +BDRVQuorumState *s = bs->opaque;
> +BdrvChild *child;
> +int i;
> +
> +for (i = 0; i < s->num_children; i++) {
> +if (s->bs[i] == child_bs) {
> +break;
> +}
> +}
> +
> +QLIST_FOREACH(child, >children, next) {
> +if (child->bs == child_bs) {
> +break;
> +}
> +}
> +
> +/* we have checked it in bdrv_del_child() */
> +assert(i < s->num_children && child);
> +
> +if (s->num_children <= s->threshold) {
> +error_setg(errp,
> +"The number of children cannot be lower than the vote threshold 
> %d",
> +s->threshold);
> +return;
> +}
> +
> +bdrv_drain(bs);
> +/* We can safely remove this child now */
> +memmove(>bs[i], >bs[i + 1],
> +(s->num_children - i - 1) * sizeof(void *));
> +s->num_children--;
> +s->bs[s->num_children] = NULL;

Same here, no one will check or use s->bs[s->num_children] so there's no
need to make it NULL.

Apart from the issue of using only part of Kevin's series, the rest are
minor things.

Thanks and sorry for the late review!

Berto

[Qemu-devel] [PATCH v5 2/4] quorum: implement bdrv_add_child() and bdrv_del_child()

[Wen Congyang]

Signed-off-by: Wen Congyang 
Signed-off-by: zhanghailiang 
Signed-off-by: Gonglei 
---
 block.c   |  6 ++---
 block/quorum.c| 72 +--
 include/block/block.h |  3 +++
 3 files changed, 76 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 1b25e43..01f6d69 100644
--- a/block.c
+++ b/block.c
@@ -1079,9 +1079,9 @@ static int bdrv_fill_options(QDict **options, const char 
**pfilename,
 return 0;
 }
 
-static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
-BlockDriverState *child_bs,
-const BdrvChildRole *child_role)
+BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
+ BlockDriverState *child_bs,
+ const BdrvChildRole *child_role)
 {
 BdrvChild *child = g_new(BdrvChild, 1);
 *child = (BdrvChild) {
diff --git a/block/quorum.c b/block/quorum.c
index 8fe53b4..111a57b 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -66,6 +66,9 @@ typedef struct QuorumVotes {
 typedef struct BDRVQuorumState {
 BlockDriverState **bs; /* children BlockDriverStates */
 int num_children;  /* children count */
+int max_children;  /* The maximum children count, we need to reallocate
+* bs if num_children grows larger than maximum.
+*/
 int threshold; /* if less than threshold children reads gave the
 * same result a quorum error occurs.
 */
@@ -874,9 +877,9 @@ static int quorum_open(BlockDriverState *bs, QDict 
*options, int flags,
 ret = -EINVAL;
 goto exit;
 }
-if (s->num_children < 2) {
+if (s->num_children < 1) {
 error_setg(_err,
-   "Number of provided children must be greater than 1");
+   "Number of provided children must be 1 or more");
 ret = -EINVAL;
 goto exit;
 }
@@ -925,6 +928,7 @@ static int quorum_open(BlockDriverState *bs, QDict 
*options, int flags,
 /* allocate the children BlockDriverState array */
 s->bs = g_new0(BlockDriverState *, s->num_children);
 opened = g_new0(bool, s->num_children);
+s->max_children = s->num_children;
 
 for (i = 0; i < s->num_children; i++) {
 char indexstr[32];
@@ -995,6 +999,67 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
 }
 }
 
+static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
+ Error **errp)
+{
+BDRVQuorumState *s = bs->opaque;
+
+bdrv_drain(bs);
+
+if (s->num_children == s->max_children) {
+if (s->max_children >= INT_MAX) {
+error_setg(errp, "Too many children");
+return;
+}
+
+s->bs = g_renew(BlockDriverState *, s->bs, s->max_children + 1);
+s->bs[s->num_children] = NULL;
+s->max_children++;
+}
+
+bdrv_ref(child_bs);
+bdrv_attach_child(bs, child_bs, _format);
+s->bs[s->num_children++] = child_bs;
+}
+
+static void quorum_del_child(BlockDriverState *bs, BlockDriverState *child_bs,
+ Error **errp)
+{
+BDRVQuorumState *s = bs->opaque;
+BdrvChild *child;
+int i;
+
+for (i = 0; i < s->num_children; i++) {
+if (s->bs[i] == child_bs) {
+break;
+}
+}
+
+QLIST_FOREACH(child, >children, next) {
+if (child->bs == child_bs) {
+break;
+}
+}
+
+/* we have checked it in bdrv_del_child() */
+assert(i < s->num_children && child);
+
+if (s->num_children <= s->threshold) {
+error_setg(errp,
+"The number of children cannot be lower than the vote threshold 
%d",
+s->threshold);
+return;
+}
+
+bdrv_drain(bs);
+/* We can safely remove this child now */
+memmove(>bs[i], >bs[i + 1],
+(s->num_children - i - 1) * sizeof(void *));
+s->num_children--;
+s->bs[s->num_children] = NULL;
+bdrv_unref_child(bs, child);
+}
+
 static void quorum_refresh_filename(BlockDriverState *bs)
 {
 BDRVQuorumState *s = bs->opaque;
@@ -1049,6 +1114,9 @@ static BlockDriver bdrv_quorum = {
 .bdrv_detach_aio_context= quorum_detach_aio_context,
 .bdrv_attach_aio_context= quorum_attach_aio_context,
 
+.bdrv_add_child = quorum_add_child,
+.bdrv_del_child = quorum_del_child,
+
 .is_filter  = true,
 .bdrv_recurse_is_first_non_filter   = quorum_recurse_is_first_non_filter,
 };
diff --git a/include/block/block.h b/include/block/block.h
index 665c56f..bd97399 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -514,6 +514,9 @@ void bdrv_disable_copy_on_read(BlockDriverState