[Qemu-devel] [PATCH v6 0/3] modify vote rules for flush operation

[Qemu-devel] [PATCH v6 0/3] modify vote rules for flush operation

[Changlong Xie]

ChangLog:
v6:
1. Make "type" mandatory for [PATCH 2/3] 
v5:
1. Fix invalid node name in docs/qmp-events.txt
2. Address comments from Eric, drop QUORUM_FLUSH_ERROR. Instead of reworking
QUORUM_REPORT_BAD to make it compatible with quorum read/write/flush operations
v4:
1. Introduce QUORUM_FLUSH_ERROR event to notify flush failure.
v3:
1. *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 (3):
  docs: fix invalid node name in qmp event
  qmp event: Refactor QUORUM_REPORT_BAD
  quorum: modify vote rules for flush operation

 block/quorum.c      | 38 ++++++++++++++++++++++++++++----------
 docs/qmp-events.txt | 11 ++++++++++-
 qapi/block.json     | 16 ++++++++++++++++
 qapi/event.json     |  4 +++-
 4 files changed, 57 insertions(+), 12 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v6 1/3] docs: fix invalid node name in qmp event

[Changlong Xie]

Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 docs/qmp-events.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index b6e8937..afb5e20 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -335,7 +335,7 @@ Data:
 Example:
 
 { "event": "QUORUM_REPORT_BAD",
-     "data": { "node-name": "1.raw", "sector-num": 345435, "sectors-count": 5 },
+     "data": { "node-name": "node0", "sector-num": 345435, "sectors-count": 5 },
      "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
 
 Note: this event is rate-limited.
-- 
1.9.3

[Qemu-devel] [PATCH v6 2/3] qmp event: Refactor QUORUM_REPORT_BAD

[Changlong Xie]

Introduce QuorumOpType, and make QUORUM_REPORT_BAD compatible
with it.

Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
---
 block/quorum.c      | 17 ++++++++++++-----
 docs/qmp-events.txt | 11 ++++++++++-
 qapi/block.json     | 16 ++++++++++++++++
 qapi/event.json     |  4 +++-
 4 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index f78d4cb..974fff9 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -218,14 +218,16 @@ static QuorumAIOCB *quorum_aio_get(BDRVQuorumState *s,
     return acb;
 }
 
-static void quorum_report_bad(QuorumAIOCB *acb, char *node_name, int ret)
+static void quorum_report_bad(QuorumOpType type, uint64_t sector_num,
+                              int nb_sectors, char *node_name, int ret)
 {
     const char *msg = NULL;
     if (ret < 0) {
         msg = strerror(-ret);
     }
-    qapi_event_send_quorum_report_bad(!!msg, msg, node_name,
-                                      acb->sector_num, acb->nb_sectors, &error_abort);
+
+    qapi_event_send_quorum_report_bad(type, !!msg, msg, node_name,
+                                      sector_num, nb_sectors, &error_abort);
 }
 
 static void quorum_report_failure(QuorumAIOCB *acb)
@@ -285,6 +287,7 @@ static void quorum_aio_cb(void *opaque, int ret)
     QuorumChildRequest *sacb = opaque;
     QuorumAIOCB *acb = sacb->parent;
     BDRVQuorumState *s = acb->common.bs->opaque;
+    QuorumOpType type;
     bool rewrite = false;
 
     if (acb->is_read && s->read_pattern == QUORUM_READ_PATTERN_FIFO) {
@@ -303,12 +306,14 @@ static void quorum_aio_cb(void *opaque, int ret)
         return;
     }
 
+    type = acb->is_read ? QUORUM_OP_TYPE_READ : QUORUM_OP_TYPE_WRITE;
     sacb->ret = ret;
     acb->count++;
     if (ret == 0) {
         acb->success_count++;
     } else {
-        quorum_report_bad(acb, sacb->aiocb->bs->node_name, ret);
+        quorum_report_bad(type, acb->sector_num, acb->nb_sectors,
+                          sacb->aiocb->bs->node_name, ret);
     }
     assert(acb->count <= s->num_children);
     assert(acb->success_count <= s->num_children);
@@ -341,7 +346,9 @@ static void quorum_report_bad_versions(BDRVQuorumState *s,
             continue;
         }
         QLIST_FOREACH(item, &version->items, next) {
-            quorum_report_bad(acb, s->children[item->index]->bs->node_name, 0);
+            quorum_report_bad(QUORUM_OP_TYPE_READ, acb->sector_num,
+                              acb->nb_sectors,
+                              s->children[item->index]->bs->node_name, 0);
         }
     }
 }
diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index afb5e20..96baacb 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -323,6 +323,7 @@ Emitted to report a corruption of a Quorum file.
 
 Data:
 
+- "type":          Quorum operation type
 - "error":         Error message (json-string, optional)
                    Only present on failure.  This field contains a human-readable
                    error message.  There are no semantics other than that the
@@ -334,10 +335,18 @@ Data:
 
 Example:
 
+Read/Write operation:
 { "event": "QUORUM_REPORT_BAD",
-     "data": { "node-name": "node0", "sector-num": 345435, "sectors-count": 5 },
+     "data": { "node-name": "node0", "sector-num": 345435, "sectors-count": 5,
+     "type": "read" },
      "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
 
+Flush operation:
+{ "event": "QUORUM_REPORT_BAD",
+     "data": { "node-name": "node0", "sector-num": 0, "sectors-count": 2097120,
+     "type": "flush", "error": "Broken pipe" },
+     "timestamp": { "seconds": 1456406829, "microseconds": 291763 } }
+
 Note: this event is rate-limited.
 
 RESET
diff --git a/qapi/block.json b/qapi/block.json
index 58e6b30..937337d 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -196,3 +196,19 @@
 ##
 { 'event': 'DEVICE_TRAY_MOVED',
   'data': { 'device': 'str', 'tray-open': 'bool' } }
+
+##
+# @QuorumOpType
+#
+# An enumeration of the quorum operation types
+#
+# @read: read operation
+#
+# @write: write operation
+#
+# @flush: flush operation
+#
+# Since: 2.6
+##
+{ 'enum': 'QuorumOpType',
+  'data': [ 'read', 'write', 'flush' ] }
diff --git a/qapi/event.json b/qapi/event.json
index cfcc887..a2b8db6 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -340,6 +340,8 @@
 #
 # Emitted to report a corruption of a Quorum file
 #
+# @type: quorum operation type (Since 2.6)
+#
 # @error: #optional, error message. Only present on failure. This field
 #         contains a human-readable error message. There are no semantics other
 #         than that the block layer reported an error and clients should not
@@ -354,7 +356,7 @@
 # Since: 2.0
 ##
 { 'event': 'QUORUM_REPORT_BAD',
-  'data': { '*error': 'str', 'node-name': 'str',
+  'data': { 'type': 'QuorumOpType', '*error': 'str', 'node-name': 'str',
             'sector-num': 'int', 'sectors-count': 'int' } }
 
 ##
-- 
1.9.3

[Qemu-devel] [PATCH v6 3/3] 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

Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/quorum.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index 974fff9..ed906d0 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -770,19 +770,30 @@ 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) {
+            quorum_report_bad(QUORUM_OP_TYPE_FLUSH, 0,
+                              bdrv_nb_sectors(s->children[i]->bs),
+                              s->children[i]->bs->node_name, 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 v6 2/3] qmp event: Refactor QUORUM_REPORT_BAD

[Alberto Garcia]

On Thu 25 Feb 2016 06:33:08 AM CET, Changlong Xie <xiecl.fnst@cn.fujitsu.com> wrote:
> +Read/Write operation:
>  { "event": "QUORUM_REPORT_BAD",
> -     "data": { "node-name": "node0", "sector-num": 345435, "sectors-count": 5 },
> +     "data": { "node-name": "node0", "sector-num": 345435, "sectors-count": 5,
> +     "type": "read" },
>       "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }

Since you introduced the 'type' field and this is now an example of a
read error, you can change the description to say simply "Read
operation:". In my opinion there's no need to add yet another example
for a write operation, I think it's clear enough.

> +Flush operation:
> +{ "event": "QUORUM_REPORT_BAD",
> +     "data": { "node-name": "node0", "sector-num": 0, "sectors-count": 2097120,
> +     "type": "flush", "error": "Broken pipe" },
> +     "timestamp": { "seconds": 1456406829, "microseconds": 291763 } }

Here (and in the previous case) please indent "type" so it goes under
"node-name":

   { "event": "QUORUM_REPORT_BAD",
     "data": { "node-name": "node0", "sector-num": 0, "sectors-count": 2097120,
               "type": "flush", "error": "Broken pipe" },
     "timestamp": { "seconds": 1456406829, "microseconds": 291763 } }

Otherwise I think the patch looks perfect now. Thanks!

Berto

Re: [Qemu-devel] [PATCH v6 2/3] qmp event: Refactor QUORUM_REPORT_BAD

[Changlong Xie]

On 02/25/2016 06:57 PM, Alberto Garcia wrote:
> On Thu 25 Feb 2016 06:33:08 AM CET, Changlong Xie <xiecl.fnst@cn.fujitsu.com> wrote:
>> +Read/Write operation:
>>   { "event": "QUORUM_REPORT_BAD",
>> -     "data": { "node-name": "node0", "sector-num": 345435, "sectors-count": 5 },
>> +     "data": { "node-name": "node0", "sector-num": 345435, "sectors-count": 5,
>> +     "type": "read" },
>>        "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
>
> Since you introduced the 'type' field and this is now an example of a
> read error, you can change the description to say simply "Read
> operation:". In my opinion there's no need to add yet another example
> for a write operation, I think it's clear enough.

Ok

>
>> +Flush operation:
>> +{ "event": "QUORUM_REPORT_BAD",
>> +     "data": { "node-name": "node0", "sector-num": 0, "sectors-count": 2097120,
>> +     "type": "flush", "error": "Broken pipe" },
>> +     "timestamp": { "seconds": 1456406829, "microseconds": 291763 } }
>
> Here (and in the previous case) please indent "type" so it goes under

Surely.

> "node-name":
>
>     { "event": "QUORUM_REPORT_BAD",
>       "data": { "node-name": "node0", "sector-num": 0, "sectors-count": 2097120,
>                 "type": "flush", "error": "Broken pipe" },
>       "timestamp": { "seconds": 1456406829, "microseconds": 291763 } }
>
> Otherwise I think the patch looks perfect now. Thanks!
>

Thanks for your review. Will send another series.

Thanks
	-Xie

> Berto
>
>
> .
>