From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54621) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ayhZH-0000AI-UG for qemu-devel@nongnu.org; Fri, 06 May 2016 11:21:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ayhZ5-0004xl-IN for qemu-devel@nongnu.org; Fri, 06 May 2016 11:21:22 -0400 References: <1460536389-9161-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1460536389-9161-3-git-send-email-xiecl.fnst@cn.fujitsu.com> From: Max Reitz Message-ID: <13d616b1-6e84-7700-0b51-08be5f65298f@redhat.com> Date: Fri, 6 May 2016 17:20:34 +0200 MIME-Version: 1.0 In-Reply-To: <1460536389-9161-3-git-send-email-xiecl.fnst@cn.fujitsu.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="TxplGgl73msbkxi9Cg5stcU37dsTRm0ak" Subject: Re: [Qemu-devel] [PATCH v13 2/3] quorum: implement bdrv_add_child() and bdrv_del_child() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Changlong Xie , qemu devel , Eric Blake , Alberto Garcia , Kevin Wolf , Stefan Hajnoczi Cc: Markus Armbruster , "Dr. David Alan Gilbert" , Dong Eddie , Jiang Yunhong , Wen Congyang , qemu block This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --TxplGgl73msbkxi9Cg5stcU37dsTRm0ak Content-Type: multipart/mixed; boundary="H011UadQ2QSDAoAOvMVqwi1FmiUGlgsGc" From: Max Reitz To: Changlong Xie , qemu devel , Eric Blake , Alberto Garcia , Kevin Wolf , Stefan Hajnoczi Cc: Markus Armbruster , "Dr. David Alan Gilbert" , Dong Eddie , Jiang Yunhong , Wen Congyang , qemu block Message-ID: <13d616b1-6e84-7700-0b51-08be5f65298f@redhat.com> Subject: Re: [PATCH v13 2/3] quorum: implement bdrv_add_child() and bdrv_del_child() References: <1460536389-9161-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1460536389-9161-3-git-send-email-xiecl.fnst@cn.fujitsu.com> In-Reply-To: <1460536389-9161-3-git-send-email-xiecl.fnst@cn.fujitsu.com> --H011UadQ2QSDAoAOvMVqwi1FmiUGlgsGc Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 13.04.2016 10:33, Changlong Xie wrote: > From: Wen Congyang >=20 > Signed-off-by: Wen Congyang > Signed-off-by: zhanghailiang > Signed-off-by: Gonglei > Signed-off-by: Changlong Xie > --- > block.c | 8 +++--- > block/quorum.c | 78 +++++++++++++++++++++++++++++++++++++++++++= ++++++-- > include/block/block.h | 4 +++ > 3 files changed, 84 insertions(+), 6 deletions(-) Design-wise: Nice change. It's a bit strange now to have gaps in the child naming, but this strategy is very simple to implement and the order of the children used for FIFO is the same as the numerical order of the indices in the child names, so I'm happy. > diff --git a/block.c b/block.c > index 68cd3b2..4bdc6b3 100644 > --- a/block.c > +++ b/block.c > @@ -1176,10 +1176,10 @@ BdrvChild *bdrv_root_attach_child(BlockDriverSt= ate *child_bs, > return child; > } > =20 > -static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, > - BlockDriverState *child_bs, > - const char *child_name, > - const BdrvChildRole *child_role) > +BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, > + BlockDriverState *child_bs, > + const char *child_name, > + const BdrvChildRole *child_role) > { > BdrvChild *child =3D bdrv_root_attach_child(child_bs, child_name, = child_role); > QLIST_INSERT_HEAD(&parent_bs->children, child, next); > diff --git a/block/quorum.c b/block/quorum.c > index da15465..2553f82 100644 > --- a/block/quorum.c > +++ b/block/quorum.c > @@ -14,6 +14,7 @@ > */ > =20 > #include "qemu/osdep.h" > +#include "qemu/cutils.h" > #include "block/block_int.h" > #include "qapi/qmp/qbool.h" > #include "qapi/qmp/qdict.h" > @@ -67,6 +68,9 @@ typedef struct QuorumVotes { > typedef struct BDRVQuorumState { > BdrvChild **children; /* children BlockDriverStates */ > int num_children; /* children count */ > + uint64_t last_index; /* indicate the child role name of the last= > + * element of children array The name (and the comment) is a bit misleading, as it's not the index of the last child but one after that, i.e. the index of the next child should it be added. So maybe this variable should be called "next_child_index" or something similar, and the comment should reflect that. > + */ > int threshold; /* if less than threshold children reads ga= ve the > * same result a quorum error occurs. > */ > @@ -898,9 +902,9 @@ static int quorum_open(BlockDriverState *bs, QDict = *options, int flags, > ret =3D -EINVAL; > goto exit; > } > - if (s->num_children < 2) { > + if (s->num_children < 1) { > error_setg(&local_err, > - "Number of provided children must be greater than 1= "); > + "Number of provided children must be 1 or more"); > ret =3D -EINVAL; > goto exit; > } > @@ -964,6 +968,7 @@ static int quorum_open(BlockDriverState *bs, QDict = *options, int flags, > =20 > opened[i] =3D true; > } > + s->last_index =3D s->num_children; > =20 > g_free(opened); > goto exit; > @@ -1020,6 +1025,72 @@ static void quorum_attach_aio_context(BlockDrive= rState *bs, > } > } > =20 > +static void quorum_add_child(BlockDriverState *bs, BlockDriverState *c= hild_bs, > + Error **errp) > +{ > + BDRVQuorumState *s =3D bs->opaque; > + BdrvChild *child; > + char indexstr[32]; > + int ret; > + > + assert(s->num_children <=3D INT_MAX / sizeof(BdrvChild *) && > + s->last_index <=3D UINT64_MAX); > + if (s->num_children =3D=3D INT_MAX / sizeof(BdrvChild *) || > + s->last_index =3D=3D UINT64_MAX) { > + error_setg(errp, "Too many children"); > + return; > + } > + > + ret =3D snprintf(indexstr, 32, "children.%" PRIu64, s->last_index)= ; > + if (ret < 0 || ret >=3D 32) { > + error_setg(errp, "cannot generate child name"); > + return; > + } > + s->last_index++; > + > + bdrv_drain(bs); We have bdrv_drained_begin() and bdrv_drained_end() now. Perhaps we should make use of them and call bdrv_drained_begin() here... > + > + /* We can safely add the child now */ > + bdrv_ref(child_bs); > + child =3D bdrv_attach_child(bs, child_bs, indexstr, &child_format)= ; > + s->children =3D g_renew(BdrvChild *, s->children, s->num_children = + 1); > + s->children[s->num_children++] =3D child; =2E..and bdrv_drained_end() here. > +} > + > +static void quorum_del_child(BlockDriverState *bs, BdrvChild *child, > + Error **errp) > +{ > + BDRVQuorumState *s =3D bs->opaque; > + int i; > + > + for (i =3D 0; i < s->num_children; i++) { > + if (s->children[i] =3D=3D child) { > + break; > + } > + } > + > + /* we have checked it in bdrv_del_child() */ > + assert(i < s->num_children); > + > + if (s->num_children <=3D s->threshold) { > + error_setg(errp, > + "The number of children cannot be lower than the vote thre= shold %d", > + s->threshold); > + return; > + } > + > + /* child->name is "children.%d" */ > + assert(!strncmp(child->name, "children.", 9)); This could be removed now, too. I was asking for these assertions (of which in this version only this one is left) because we were using the child name to infer the index in the child bitmap before. Now we're not using the child name here at all, so we can just drop this assertion. > + > + bdrv_drain(bs); As above, bdrv_drained_begin() here... > + > + /* We can safely remove this child now */ > + memmove(&s->children[i], &s->children[i + 1], > + (s->num_children - i - 1) * sizeof(BdrvChild *)); > + s->children =3D g_renew(BdrvChild *, s->children, --s->num_childre= n); > + bdrv_unref_child(bs, child); =2E..and bdrv_drained_end() here may be better than a plain bdrv_drain().= > +} > + > static void quorum_refresh_filename(BlockDriverState *bs, QDict *optio= ns) > { > BDRVQuorumState *s =3D bs->opaque; > @@ -1075,6 +1146,9 @@ static BlockDriver bdrv_quorum =3D { > .bdrv_detach_aio_context =3D quorum_detach_aio_context,= > .bdrv_attach_aio_context =3D quorum_attach_aio_context,= > =20 > + .bdrv_add_child =3D quorum_add_child, > + .bdrv_del_child =3D quorum_del_child, > + > .is_filter =3D true, > .bdrv_recurse_is_first_non_filter =3D quorum_recurse_is_first_no= n_filter, > }; > diff --git a/include/block/block.h b/include/block/block.h > index 694ca76..52902cd 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -476,6 +476,10 @@ void bdrv_disable_copy_on_read(BlockDriverState *b= s); > void bdrv_ref(BlockDriverState *bs); > void bdrv_unref(BlockDriverState *bs); > void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child); > +BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, > + BlockDriverState *child_bs, > + const char *child_name, > + const BdrvChildRole *child_role); > =20 > bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **= errp); > void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason= ); >=20 None of the things I pointed out above is critical, but I'd still rather see them fixed before giving my R-b. Max --H011UadQ2QSDAoAOvMVqwi1FmiUGlgsGc-- --TxplGgl73msbkxi9Cg5stcU37dsTRm0ak Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXLLZCAAoJEDuxQgLoOKytWJgH/38w5M7607iMSy1+ykwqX/gy MEwIA+n4SjZcMC6cqSMkQgzHfaZQv7Ua8kU2OtFw5tgcKcy3eeQnlEOJv0uBu7eJ wlCsVr/WlYCjJG6EHwcJ9S8tZvOXe4Ep6nCW6nhwCDueALk8YqTDPHWZnsfyTAja Cw8C5+buICRsHxYHnqYw8ZIEZS7JBuHpD4XhHnxnlMegSS0uXSYQ2mU4mCC4dWLO tyatQhTbPD8VostD8YfhPphvxn8G5gJcR0pEdjBMZo6Z2o3tawHqo6+q+3vCuSGx YpcYcQtqtq6Y0oaIAxQiF3bSHibvuANhBELkVty7C8eJQIVBImeqp//BlD7MFII= =7bBC -----END PGP SIGNATURE----- --TxplGgl73msbkxi9Cg5stcU37dsTRm0ak--