From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38385) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1azhQt-0007Ra-Sf for qemu-devel@nongnu.org; Mon, 09 May 2016 05:24:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1azhQr-0006Aw-4y for qemu-devel@nongnu.org; Mon, 09 May 2016 05:24:54 -0400 Message-ID: <573057D9.9060109@cn.fujitsu.com> Date: Mon, 9 May 2016 17:26:49 +0800 From: Changlong Xie MIME-Version: 1.0 References: <1460536389-9161-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1460536389-9161-3-git-send-email-xiecl.fnst@cn.fujitsu.com> <13d616b1-6e84-7700-0b51-08be5f65298f@redhat.com> In-Reply-To: <13d616b1-6e84-7700-0b51-08be5f65298f@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit 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: Max Reitz , 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 On 05/06/2016 11:20 PM, Max Reitz wrote: > On 13.04.2016 10:33, Changlong Xie wrote: >> From: Wen Congyang >> >> 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. I think this is the simplest approach too : ) > >> 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(BlockDriverState *child_bs, >> return child; >> } >> >> -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 = 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 @@ >> */ >> >> #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. > Will fix. >> + */ >> int threshold; /* if less than threshold children reads gave the >> * same result a quorum error occurs. >> */ >> @@ -898,9 +902,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(&local_err, >> - "Number of provided children must be greater than 1"); >> + "Number of provided children must be 1 or more"); >> ret = -EINVAL; >> goto exit; >> } >> @@ -964,6 +968,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags, >> >> opened[i] = true; >> } >> + s->last_index = s->num_children; >> >> g_free(opened); >> goto exit; >> @@ -1020,6 +1025,72 @@ static void quorum_attach_aio_context(BlockDriverState *bs, >> } >> } >> >> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs, >> + Error **errp) >> +{ >> + BDRVQuorumState *s = bs->opaque; >> + BdrvChild *child; >> + char indexstr[32]; >> + int ret; >> + >> + assert(s->num_children <= INT_MAX / sizeof(BdrvChild *) && >> + s->last_index <= UINT64_MAX); >> + if (s->num_children == INT_MAX / sizeof(BdrvChild *) || >> + s->last_index == UINT64_MAX) { >> + error_setg(errp, "Too many children"); >> + return; >> + } >> + >> + ret = snprintf(indexstr, 32, "children.%" PRIu64, s->last_index); >> + if (ret < 0 || ret >= 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... Ditto > >> + >> + /* We can safely add the child now */ >> + bdrv_ref(child_bs); >> + child = bdrv_attach_child(bs, child_bs, indexstr, &child_format); >> + s->children = g_renew(BdrvChild *, s->children, s->num_children + 1); >> + s->children[s->num_children++] = child; > > ...and bdrv_drained_end() here. Ditto > >> +} >> + >> +static void quorum_del_child(BlockDriverState *bs, BdrvChild *child, >> + Error **errp) >> +{ >> + BDRVQuorumState *s = bs->opaque; >> + int i; >> + >> + for (i = 0; i < s->num_children; i++) { >> + if (s->children[i] == child) { >> + break; >> + } >> + } >> + >> + /* we have checked it in bdrv_del_child() */ >> + assert(i < s->num_children); >> + >> + if (s->num_children <= s->threshold) { >> + error_setg(errp, >> + "The number of children cannot be lower than the vote threshold %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. Will remove it in next version. > >> + >> + bdrv_drain(bs); > > As above, bdrv_drained_begin() here... Will fix it. > >> + >> + /* We can safely remove this child now */ >> + memmove(&s->children[i], &s->children[i + 1], >> + (s->num_children - i - 1) * sizeof(BdrvChild *)); >> + s->children = g_renew(BdrvChild *, s->children, --s->num_children); >> + bdrv_unref_child(bs, child); > > ...and bdrv_drained_end() here may be better than a plain bdrv_drain(). > Ditto. >> +} >> + >> static void quorum_refresh_filename(BlockDriverState *bs, QDict *options) >> { >> BDRVQuorumState *s = bs->opaque; >> @@ -1075,6 +1146,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 694ca76..52902cd 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -476,6 +476,10 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs); >> 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); >> >> bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp); >> void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason); >> > > None of the things I pointed out above is critical, but I'd still rather > see them fixed before giving my R-b. I'm glab to hear that, will send out next version. Thanks -Xie > > Max >