[V10,10/13] quorum: Add quorum_co_flush().

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(+)

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