Message ID | 1390927974-31325-11-git-send-email-benoit.canet@irqsave.net |
---|---|
State | New |
Headers | show |
On 28.01.2014 17:52, Benoît Canet wrote: > From: Benoît Canet <benoit@irqsave.net> > > Makes a vote to select error if any. > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > --- > block/quorum.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/block/quorum.c b/block/quorum.c > index 9b0718b..1b84b07 100644 > --- a/block/quorum.c > +++ b/block/quorum.c > @@ -653,12 +653,46 @@ free_exit: > return result; > } > > +static coroutine_fn int quorum_co_flush(BlockDriverState *bs) > +{ > + BDRVQuorumState *s = bs->opaque; > + QuorumVoteVersion *winner = NULL; > + QuorumVotes error_votes; > + QuorumVoteValue result_value; > + int i; > + int result = 0; > + bool error = false; > + > + QLIST_INIT(&error_votes.vote_list); > + error_votes.compare = quorum_64bits_compare; > + > + for (i = 0; i < s->total; i++) { > + result = bdrv_co_flush(s->bs[i]); > + if (result) { > + error = true; > + result_value.l = result; > + quorum_count_vote(&error_votes, &result_value, i); > + } > + } > + > + if (error) { > + winner = quorum_get_vote_winner(&error_votes); > + result = winner->value.l; > + } > + > + quorum_free_vote_list(&error_votes); > + > + return result; > +} > + > static BlockDriver bdrv_quorum = { > .format_name = "quorum", > .protocol_name = "quorum", > > .instance_size = sizeof(BDRVQuorumState), > > + .bdrv_co_flush_to_disk = quorum_co_flush, > + > .bdrv_getlength = quorum_getlength, > > .bdrv_aio_readv = quorum_aio_readv, So, my general opinion on this patch (for reads/writes we don't vote on the error code either; so why here?) hasn't changed, but well, I definitely don't oppose it. Another problem, however: If any error occurs, this function will return an error as well. Is that intended? If an error on a read/write operation occurs but there are still enough successful reads/writes to reach quorum, no error is returned. Is there a reason why this should be different for flush? Max
Le Sunday 02 Feb 2014 à 23:02:57 (+0100), Max Reitz a écrit : > On 28.01.2014 17:52, Benoît Canet wrote: > >From: Benoît Canet <benoit@irqsave.net> > > > >Makes a vote to select error if any. > > > >Signed-off-by: Benoit Canet <benoit@irqsave.net> > >--- > > block/quorum.c | 34 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > >diff --git a/block/quorum.c b/block/quorum.c > >index 9b0718b..1b84b07 100644 > >--- a/block/quorum.c > >+++ b/block/quorum.c > >@@ -653,12 +653,46 @@ free_exit: > > return result; > > } > >+static coroutine_fn int quorum_co_flush(BlockDriverState *bs) > >+{ > >+ BDRVQuorumState *s = bs->opaque; > >+ QuorumVoteVersion *winner = NULL; > >+ QuorumVotes error_votes; > >+ QuorumVoteValue result_value; > >+ int i; > >+ int result = 0; > >+ bool error = false; > >+ > >+ QLIST_INIT(&error_votes.vote_list); > >+ error_votes.compare = quorum_64bits_compare; > >+ > >+ for (i = 0; i < s->total; i++) { > >+ result = bdrv_co_flush(s->bs[i]); > >+ if (result) { > >+ error = true; > >+ result_value.l = result; > >+ quorum_count_vote(&error_votes, &result_value, i); > >+ } > >+ } > >+ > >+ if (error) { > >+ winner = quorum_get_vote_winner(&error_votes); > >+ result = winner->value.l; > >+ } > >+ > >+ quorum_free_vote_list(&error_votes); > >+ > >+ return result; > >+} > >+ > > static BlockDriver bdrv_quorum = { > > .format_name = "quorum", > > .protocol_name = "quorum", > > .instance_size = sizeof(BDRVQuorumState), > >+ .bdrv_co_flush_to_disk = quorum_co_flush, > >+ > > .bdrv_getlength = quorum_getlength, > > .bdrv_aio_readv = quorum_aio_readv, > > So, my general opinion on this patch (for reads/writes we don't vote > on the error code either; so why here?) hasn't changed, but well, I > definitely don't oppose it. For the reads/writes Kevin asked me to return the first error. For the flush function someone else (probably Eric) asked me to do voting. > > Another problem, however: If any error occurs, this function will > return an error as well. Is that intended? If an error on a > read/write operation occurs but there are still enough successful > reads/writes to reach quorum, no error is returned. Is there a > reason why this should be different for flush? If an error occurs multiple times and is able to establish quorum this winning error will be returned. Else 0 will be returned due to the vote result if the error is in minority. So the behavior is similar as long quorum is reached. > > Max
On 03.02.2014 12:57, Benoît Canet wrote: > Le Sunday 02 Feb 2014 à 23:02:57 (+0100), Max Reitz a écrit : >> On 28.01.2014 17:52, Benoît Canet wrote: >>> From: Benoît Canet <benoit@irqsave.net> >>> >>> Makes a vote to select error if any. >>> >>> Signed-off-by: Benoit Canet <benoit@irqsave.net> >>> --- >>> block/quorum.c | 34 ++++++++++++++++++++++++++++++++++ >>> 1 file changed, 34 insertions(+) >>> >>> diff --git a/block/quorum.c b/block/quorum.c >>> index 9b0718b..1b84b07 100644 >>> --- a/block/quorum.c >>> +++ b/block/quorum.c >>> @@ -653,12 +653,46 @@ free_exit: >>> return result; >>> } >>> +static coroutine_fn int quorum_co_flush(BlockDriverState *bs) >>> +{ >>> + BDRVQuorumState *s = bs->opaque; >>> + QuorumVoteVersion *winner = NULL; >>> + QuorumVotes error_votes; >>> + QuorumVoteValue result_value; >>> + int i; >>> + int result = 0; >>> + bool error = false; >>> + >>> + QLIST_INIT(&error_votes.vote_list); >>> + error_votes.compare = quorum_64bits_compare; >>> + >>> + for (i = 0; i < s->total; i++) { >>> + result = bdrv_co_flush(s->bs[i]); >>> + if (result) { >>> + error = true; >>> + result_value.l = result; >>> + quorum_count_vote(&error_votes, &result_value, i); >>> + } >>> + } >>> + >>> + if (error) { >>> + winner = quorum_get_vote_winner(&error_votes); >>> + result = winner->value.l; >>> + } >>> + >>> + quorum_free_vote_list(&error_votes); >>> + >>> + return result; >>> +} >>> + >>> static BlockDriver bdrv_quorum = { >>> .format_name = "quorum", >>> .protocol_name = "quorum", >>> .instance_size = sizeof(BDRVQuorumState), >>> + .bdrv_co_flush_to_disk = quorum_co_flush, >>> + >>> .bdrv_getlength = quorum_getlength, >>> .bdrv_aio_readv = quorum_aio_readv, >> So, my general opinion on this patch (for reads/writes we don't vote >> on the error code either; so why here?) hasn't changed, but well, I >> definitely don't oppose it. > For the reads/writes Kevin asked me to return the first error. > For the flush function someone else (probably Eric) asked me to do voting. > >> Another problem, however: If any error occurs, this function will >> return an error as well. Is that intended? If an error on a >> read/write operation occurs but there are still enough successful >> reads/writes to reach quorum, no error is returned. Is there a >> reason why this should be different for flush? > If an error occurs multiple times and is able to establish quorum this winning > error will be returned. > Else 0 will be returned due to the vote result if the error is in minority. > So the behavior is similar as long quorum is reached. You're only counting results which are not 0. Therefore, 0 will never reach majority. Max >> Max
diff --git a/block/quorum.c b/block/quorum.c index 9b0718b..1b84b07 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -653,12 +653,46 @@ free_exit: return result; } +static coroutine_fn int quorum_co_flush(BlockDriverState *bs) +{ + BDRVQuorumState *s = bs->opaque; + QuorumVoteVersion *winner = NULL; + QuorumVotes error_votes; + QuorumVoteValue result_value; + int i; + int result = 0; + bool error = false; + + QLIST_INIT(&error_votes.vote_list); + error_votes.compare = quorum_64bits_compare; + + for (i = 0; i < s->total; i++) { + result = bdrv_co_flush(s->bs[i]); + if (result) { + error = true; + result_value.l = result; + quorum_count_vote(&error_votes, &result_value, i); + } + } + + if (error) { + winner = quorum_get_vote_winner(&error_votes); + result = winner->value.l; + } + + quorum_free_vote_list(&error_votes); + + return result; +} + static BlockDriver bdrv_quorum = { .format_name = "quorum", .protocol_name = "quorum", .instance_size = sizeof(BDRVQuorumState), + .bdrv_co_flush_to_disk = quorum_co_flush, + .bdrv_getlength = quorum_getlength, .bdrv_aio_readv = quorum_aio_readv,