[Qemu-devel] [PATCH v3 0/1] modify vote rules for flush operation

[Qemu-devel] [PATCH v3 0/1] modify vote rules for flush operation

[Changlong Xie]

ChangLog:
v3:
*Note* that, the codes logic is different from what we talked in v2. I just
keep flush interface the same logic as quorum read/write, and think it's
reasoned.

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

Changlong Xie (1):
  quorum: modify vote rules for flush operation

 block/quorum.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v3 1/1] quorum: modify vote rules for flush operation

[Changlong Xie]

Keep flush interface the same logic as quorum read/write, Otherwise in
following scenario, we'll encounter unexpected errors.

Quorum has two children(A, B). A do flush sucessfully, but B flush failed.
This cause the filesystem of guest become read-only with following errors:

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

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

diff --git a/block/quorum.c b/block/quorum.c
index f78d4cb..62d3ccb 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -763,19 +763,26 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
     QuorumVoteValue result_value;
     int i;
     int result = 0;
+    int success_count = 0;
 
     QLIST_INIT(&error_votes.vote_list);
     error_votes.compare = quorum_64bits_compare;
 
     for (i = 0; i < s->num_children; i++) {
         result = bdrv_co_flush(s->children[i]->bs);
-        result_value.l = result;
-        quorum_count_vote(&error_votes, &result_value, i);
+        if (result) {
+            result_value.l = result;
+            quorum_count_vote(&error_votes, &result_value, i);
+        } else
+            success_count++;
     }
 
-    winner = quorum_get_vote_winner(&error_votes);
-    result = winner->value.l;
-
+    if (success_count >= s->threshold)
+        result = 0;
+    else {
+        winner = quorum_get_vote_winner(&error_votes);
+        result = winner->value.l;
+    }
     quorum_free_vote_list(&error_votes);
 
     return result;
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 1/1] quorum: modify vote rules for flush operation

[Alberto Garcia]

On Mon 22 Feb 2016 10:50:37 AM CET, Changlong Xie wrote:
> -    winner = quorum_get_vote_winner(&error_votes);
> -    result = winner->value.l;
> -
> +    if (success_count >= s->threshold)
> +        result = 0;
> +    else {
> +        winner = quorum_get_vote_winner(&error_votes);
> +        result = winner->value.l;
> +    }

Please use braces in both branches of the if. scripts/checkpatch.pl
should report that.

Other than that I think the patch is correct, but I still wonder if we
should emit QUORUM_REPORT_BAD (or a new event) for the operations that
fail. Or why wouldn't the user want to be notified of a flush failure?

Berto

Re: [Qemu-devel] [PATCH v3 1/1] quorum: modify vote rules for flush operation

[Changlong Xie]

On 02/22/2016 10:33 PM, Alberto Garcia wrote:
> On Mon 22 Feb 2016 10:50:37 AM CET, Changlong Xie wrote:
>> -    winner = quorum_get_vote_winner(&error_votes);
>> -    result = winner->value.l;
>> -
>> +    if (success_count >= s->threshold)
>> +        result = 0;
>> +    else {
>> +        winner = quorum_get_vote_winner(&error_votes);
>> +        result = winner->value.l;
>> +    }
>
> Please use braces in both branches of the if. scripts/checkpatch.pl
> should report that.

Surely.

>
> Other than that I think the patch is correct, but I still wonder if we
> should emit QUORUM_REPORT_BAD (or a new event) for the operations that

To me QUORUM_REPORT_BAD is not a good choice.

> fail. Or why wouldn't the user want to be notified of a flush failure?
>

I'll introduce a new event in next series.

Thanks
	-Xie

> Berto
>
>
> .
>