| Message ID | 1442907862-21376-3-git-send-email-wency@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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, &child_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, &bs->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(&s->bs[i], &s->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
On 22.09.2015 09:44, Wen Congyang wrote: > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > 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(&local_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) { Opposing Berto (:-)), I like >= even if the > part is actually impossible. I myself like to use constructs such as: for (i = 0; i < n; i++) { /* ... */ } if (i >= n) { /* ... */ } Even though @i can never exceed @n after the loop. This is because my way of thinking is "What if it could exceed @n? Then I'd like to take this branch as well." The same applies here. s->max_children can never exceed INT_MAX, but if it could, we'd want that to be an error, too. I do find myself in discussions about that from time to time, especially since others prefer (for the example above): for (i = 0; i < n; i++) { /* ... */ } assert(i <= n); if (i == n) { /* ... */ } Or the like. > + 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++; > + } Just a suggestion, please feel free to ignore it completely: You can drop the s->max_children field and just always call g_renew() with s->num_children + 1 as the @count parameter. There shouldn't be any (visible) performance penalty, but it would simplify the code. > + > + bdrv_ref(child_bs); > + bdrv_attach_child(bs, child_bs, &child_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, &bs->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(&s->bs[i], &s->bs[i + 1], > + (s->num_children - i - 1) * sizeof(void *)); I'd use a matching type here, i.e. sizeof(BlockDriverState *) before Kevin's bdrv_swap() series or sizeof(BdrvChild *) afterwards. > + s->num_children--; > + s->bs[s->num_children] = NULL; In case you do decide to drop s->max_children, you could replace this by a call to g_renew on s->bs (or s->children). In any case, as Berto said, this line is unnecessary. Max > + 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 *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 BdrvChildRole *child_role); > > bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp); > void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason); >
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, &child_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, &bs->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(&s->bs[i], &s->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 > . >
On Wed 07 Oct 2015 08:51:21 PM CEST, Max Reitz <mreitz@redhat.com> wrote: >> + if (s->num_children == s->max_children) { >> + if (s->max_children >= INT_MAX) { > > Opposing Berto (:-)), I like >= even if the > part is actually > impossible. I myself like to use constructs such as: > > for (i = 0; i < n; i++) { > /* ... */ > } > if (i >= n) { > /* ... */ > } > > Even though @i can never exceed @n after the loop. This is because my > way of thinking is "What if it could exceed @n? Then I'd like to take > this branch as well." The same applies here. s->max_children can never > exceed INT_MAX, but if it could, we'd want that to be an error, too. If s->max_children (and therefore s->num_children) could exceed the upper limit then we would probably want to assert on that, because it means that there's something seriously broken. The purpose of this code is to make sure that the upper limit is... well, an upper limit :-) If that invariant no longer holds then I don't think we want a simple "Too many children" error. >> + s->bs = g_renew(BlockDriverState *, s->bs, s->max_children + 1); >> + s->bs[s->num_children] = NULL; >> + s->max_children++; >> + } > > Just a suggestion, please feel free to ignore it completely: > > You can drop the s->max_children field and just always call g_renew() > with s->num_children + 1 as the @count parameter. There shouldn't be > any (visible) performance penalty, but it would simplify the code. If s->num_children has decreased since the previous g_renew() call (because the user called quorum_del_child()) that could actually reduce the array size. Nothing would break though, at worst it would just be a bit counter-intuitive :-) https://www.gnu.org/software/libc/manual/html_node/Changing-Block-Size.html Berto
On 08.10.2015 10:12, Alberto Garcia wrote: > On Wed 07 Oct 2015 08:51:21 PM CEST, Max Reitz <mreitz@redhat.com> wrote: >>> + if (s->num_children == s->max_children) { >>> + if (s->max_children >= INT_MAX) { >> >> Opposing Berto (:-)), I like >= even if the > part is actually >> impossible. I myself like to use constructs such as: >> >> for (i = 0; i < n; i++) { >> /* ... */ >> } >> if (i >= n) { >> /* ... */ >> } >> >> Even though @i can never exceed @n after the loop. This is because my >> way of thinking is "What if it could exceed @n? Then I'd like to take >> this branch as well." The same applies here. s->max_children can never >> exceed INT_MAX, but if it could, we'd want that to be an error, too. > > If s->max_children (and therefore s->num_children) could exceed the > upper limit then we would probably want to assert on that, because it > means that there's something seriously broken. The purpose of this code > is to make sure that the upper limit is... well, an upper limit :-) If > that invariant no longer holds then I don't think we want a simple "Too > many children" error. > >>> + s->bs = g_renew(BlockDriverState *, s->bs, s->max_children + 1); >>> + s->bs[s->num_children] = NULL; >>> + s->max_children++; >>> + } >> >> Just a suggestion, please feel free to ignore it completely: >> >> You can drop the s->max_children field and just always call g_renew() >> with s->num_children + 1 as the @count parameter. There shouldn't be >> any (visible) performance penalty, but it would simplify the code. > > If s->num_children has decreased since the previous g_renew() call > (because the user called quorum_del_child()) that could actually reduce > the array size. Yes, it could. And that would be just fine. ;-) We'd just keep the array exactly as big as it needs to be. I find that pretty intuitive. It's just counter-intuitive if you think one should never use realloc() for reducing the size of a buffer (and I know I myself tend to write my code thinking that). Max > Nothing would break though, at worst it would just be a > bit counter-intuitive :-) > > https://www.gnu.org/software/libc/manual/html_node/Changing-Block-Size.html > > Berto >
On Fri 09 Oct 2015 05:51:55 PM CEST, Max Reitz <mreitz@redhat.com> wrote: >>>> + s->bs = g_renew(BlockDriverState *, s->bs, s->max_children + 1); >>>> + s->bs[s->num_children] = NULL; >>>> + s->max_children++; >>>> + } >>> >>> Just a suggestion, please feel free to ignore it completely: >>> >>> You can drop the s->max_children field and just always call g_renew() >>> with s->num_children + 1 as the @count parameter. There shouldn't be >>> any (visible) performance penalty, but it would simplify the code. >> >> If s->num_children has decreased since the previous g_renew() call >> (because the user called quorum_del_child()) that could actually reduce >> the array size. > > Yes, it could. And that would be just fine. ;-) > > We'd just keep the array exactly as big as it needs to be. I find that > pretty intuitive. It's just counter-intuitive if you think one should > never use realloc() for reducing the size of a buffer (and I know I > myself tend to write my code thinking that). If the goal is to keep the array exactly as big as it needs to be then we should use g_renew() in quorum_del_child()... Anyway we're digressing :-) this array is one pointer per Quorum child, so the amount of memory we're talking about here is probably negligible. I'm fine with any solution. Berto
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(&local_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, &child_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, &bs->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(&s->bs[i], &s->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 *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 BdrvChildRole *child_role); bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp); void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);