[Qemu-devel] [PATCH 0/1] change quorum vote rules for 64-bits hash

[Qemu-devel] [PATCH 0/1] change quorum vote rules for 64-bits hash

[Changlong Xie]

If quorum has two children(A, B). A do flush sucessfully, but 
B flush failed. We MUST choice A as winner, otherwise we will 
get following errors:

{"timestamp": {"seconds": 1455641588, "microseconds": 415937}, 
"event": "BLOCK_IO_ERROR", "data": {"device": "colo-disk", 
"nospace": false, "reason": "Bad file descriptor", "operation": 
"write", "action": "report"}}

And the filesystem of guest became read-only with following errors:

[xxx] end_request: I/O error, dev vda, sector 11159960
[xxx] Aborting journal on device vda3-8
[xxx] EXT4-fs error (device vda3): ext4_journal_start_sb:327: Detected abort journal
[xxx] EXT4-fs (vda3): Remounting filesystem read-only

[Ref] http://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg05342.html 

Changlong Xie (1):
  quorum: change vote rules for 64 bits hash

 block/quorum.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/1] quorum: change vote rules for 64 bits hash

[Changlong Xie]

Before:
1) vote_count > max: winner = candidate, update max
2) vote_count <= max: do nothing
Current:
1) vote_count > max: winner = candidate, update max
2) vote_count = max: compare the value of winner with
candidate, if candidate->value.l == 0, winner = candidate,
else do nothing
3) vote_count < max: do nothing

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
---
 block/quorum.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index a5ae4b8..e431ff4 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -446,7 +446,7 @@ static int quorum_compute_hash(QuorumAIOCB *acb, int i, QuorumVoteValue *hash)
     return 0;
 }
 
-static QuorumVoteVersion *quorum_get_vote_winner(QuorumVotes *votes)
+static QuorumVoteVersion *quorum_get_vote_winner(QuorumVotes *votes, bool vote_error)
 {
     int max = 0;
     QuorumVoteVersion *candidate, *winner = NULL;
@@ -455,6 +455,12 @@ static QuorumVoteVersion *quorum_get_vote_winner(QuorumVotes *votes)
         if (candidate->vote_count > max) {
             max = candidate->vote_count;
             winner = candidate;
+            continue;
+        }
+        /* For 64 bit hash */
+        if (vote_error == true && candidate->vote_count == max
+            && candidate->value.l == 0) {
+            winner = candidate;
         }
     }
 
@@ -544,7 +550,7 @@ static int quorum_vote_error(QuorumAIOCB *acb)
     }
 
     if (error) {
-        winner = quorum_get_vote_winner(&error_votes);
+        winner = quorum_get_vote_winner(&error_votes, false);
         ret = winner->value.l;
     }
 
@@ -609,7 +615,7 @@ static bool quorum_vote(QuorumAIOCB *acb)
     }
 
     /* vote to select the most represented version */
-    winner = quorum_get_vote_winner(&acb->votes);
+    winner = quorum_get_vote_winner(&acb->votes, false);
 
     /* if the winner count is smaller than threshold the read fails */
     if (winner->vote_count < s->threshold) {
@@ -769,7 +775,7 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
         quorum_count_vote(&error_votes, &result_value, i);
     }
 
-    winner = quorum_get_vote_winner(&error_votes);
+    winner = quorum_get_vote_winner(&error_votes, true);
     result = winner->value.l;
 
     quorum_free_vote_list(&error_votes);
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/1] quorum: change vote rules for 64 bits hash

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]

On 02/15/2016 02:52 AM, Changlong Xie wrote:
> Before:
> 1) vote_count > max: winner = candidate, update max
> 2) vote_count <= max: do nothing
> Current:
> 1) vote_count > max: winner = candidate, update max
> 2) vote_count = max: compare the value of winner with
> candidate, if candidate->value.l == 0, winner = candidate,
> else do nothing
> 3) vote_count < max: do nothing

This says what you did, but not why.  Can you demonstrate a scenario
that is broken without the patch, as part of your commit message?

> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> ---
>  block/quorum.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index a5ae4b8..e431ff4 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -446,7 +446,7 @@ static int quorum_compute_hash(QuorumAIOCB *acb, int i, QuorumVoteValue *hash)
>      return 0;
>  }
>  
> -static QuorumVoteVersion *quorum_get_vote_winner(QuorumVotes *votes)
> +static QuorumVoteVersion *quorum_get_vote_winner(QuorumVotes *votes, bool vote_error)

Long line. Please wrap things to stay within 80 columns.

>  {
>      int max = 0;
>      QuorumVoteVersion *candidate, *winner = NULL;
> @@ -455,6 +455,12 @@ static QuorumVoteVersion *quorum_get_vote_winner(QuorumVotes *votes)
>          if (candidate->vote_count > max) {
>              max = candidate->vote_count;
>              winner = candidate;
> +            continue;
> +        }
> +        /* For 64 bit hash */
> +        if (vote_error == true && candidate->vote_count == max

s/ == true// (no need to do a redundant comparison of a bool against a
bool).


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/1] quorum: change vote rules for 64 bits hash

[Changlong Xie]

On 02/16/2016 01:10 AM, Eric Blake wrote:
> On 02/15/2016 02:52 AM, Changlong Xie wrote:
>> Before:
>> 1) vote_count > max: winner = candidate, update max
>> 2) vote_count <= max: do nothing
>> Current:
>> 1) vote_count > max: winner = candidate, update max
>> 2) vote_count = max: compare the value of winner with
>> candidate, if candidate->value.l == 0, winner = candidate,
>> else do nothing
>> 3) vote_count < max: do nothing
>
> This says what you did, but not why.  Can you demonstrate a scenario
> that is broken without the patch, as part of your commit message?

Surely, will do in next version.

>
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>> ---
>>   block/quorum.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/quorum.c b/block/quorum.c
>> index a5ae4b8..e431ff4 100644
>> --- a/block/quorum.c
>> +++ b/block/quorum.c
>> @@ -446,7 +446,7 @@ static int quorum_compute_hash(QuorumAIOCB *acb, int i, QuorumVoteValue *hash)
>>       return 0;
>>   }
>>
>> -static QuorumVoteVersion *quorum_get_vote_winner(QuorumVotes *votes)
>> +static QuorumVoteVersion *quorum_get_vote_winner(QuorumVotes *votes, bool vote_error)
>
> Long line. Please wrap things to stay within 80 columns.
>

Ok

>>   {
>>       int max = 0;
>>       QuorumVoteVersion *candidate, *winner = NULL;
>> @@ -455,6 +455,12 @@ static QuorumVoteVersion *quorum_get_vote_winner(QuorumVotes *votes)
>>           if (candidate->vote_count > max) {
>>               max = candidate->vote_count;
>>               winner = candidate;
>> +            continue;
>> +        }
>> +        /* For 64 bit hash */
>> +        if (vote_error == true && candidate->vote_count == max
>
> s/ == true// (no need to do a redundant comparison of a bool against a
> bool).

Ok

Thanks
	-Xie

>
>