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

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

[Changlong Xie]

ChangLog:
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      | 48 ++++++++++++++++++++++++++++++++++++++----------
 docs/qmp-events.txt | 10 +++++++++-
 qapi/block.json     | 16 ++++++++++++++++
 qapi/event.json     |  4 +++-
 4 files changed, 66 insertions(+), 12 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v5 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>
---
 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 52eb7e2..f96e120 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -319,7 +319,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 v5 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      | 28 +++++++++++++++++++++++-----
 docs/qmp-events.txt |  8 ++++++++
 qapi/block.json     | 16 ++++++++++++++++
 qapi/event.json     |  4 +++-
 4 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index 11cc60b..03d68c1 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -215,14 +215,29 @@ 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, QuorumAIOCB *acb,
+                              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);
+
+    switch (type) {
+    case QUORUM_OP_TYPE_READ:
+    case QUORUM_OP_TYPE_WRITE:
+        qapi_event_send_quorum_report_bad(false, 0, !!msg, msg, node_name,
+                                          acb->sector_num, acb->nb_sectors,
+                                          &error_abort);
+        break;
+    case QUORUM_OP_TYPE_FLUSH:
+        qapi_event_send_quorum_report_bad(true, type, !!msg, msg, node_name,
+                                          0, 0, &error_abort);
+        break;
+    default:
+        /* should never happen */
+        abort();
+    }
 }
 
 static void quorum_report_failure(QuorumAIOCB *acb)
@@ -282,6 +297,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) {
@@ -300,12 +316,13 @@ 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, sacb->aiocb->bs->node_name, ret);
     }
     assert(acb->count <= s->num_children);
     assert(acb->success_count <= s->num_children);
@@ -338,7 +355,8 @@ 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,
+                              s->children[item->index]->bs->node_name, 0);
         }
     }
 }
diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index f96e120..0a082db 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -307,6 +307,7 @@ Emitted to report a corruption of a Quorum file.
 
 Data:
 
+- "type":          Quorum operation type (json-string, optional)
 - "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
@@ -318,10 +319,17 @@ Data:
 
 Example:
 
+Read/Write operation:
 { "event": "QUORUM_REPORT_BAD",
      "data": { "node-name": "node0", "sector-num": 345435, "sectors-count": 5 },
      "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
 
+Flush operation:
+{ "event": "QUORUM_REPORT_BAD",
+     "data": { "node-name": "node0", "sectors-count": 0, "sector-num": 0,
+     "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 390fd45..a5db99a 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -325,6 +325,8 @@
 #
 # Emitted to report a corruption of a Quorum file
 #
+# @type: #optional, 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
@@ -339,7 +341,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 v5 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>
---
 block/quorum.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index 03d68c1..0c7e4ad 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -778,19 +778,29 @@ 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, NULL,
+                              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 v5 1/3] docs: fix invalid node name in qmp event

[Alberto Garcia]

On Wed 24 Feb 2016 11:11:53 AM CET, Changlong Xie <xiecl.fnst@cn.fujitsu.com> wrote:
> 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>

Berto

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

[Alberto Garcia]

On Wed 24 Feb 2016 11:11:54 AM CET, Changlong Xie wrote:
> -static void quorum_report_bad(QuorumAIOCB *acb, char *node_name, int ret)
> +static void quorum_report_bad(QuorumOpType type, QuorumAIOCB *acb,
> +                              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);
> +
> +    switch (type) {
> +    case QUORUM_OP_TYPE_READ:
> +    case QUORUM_OP_TYPE_WRITE:
> +        qapi_event_send_quorum_report_bad(false, 0, !!msg, msg, node_name,
> +                                          acb->sector_num, acb->nb_sectors,
> +                                          &error_abort);
> +        break;
> +    case QUORUM_OP_TYPE_FLUSH:
> +        qapi_event_send_quorum_report_bad(true, type, !!msg, msg, node_name,
> +                                          0, 0, &error_abort);
> +        break;

A few comments:

  - Why don't you set the 'type' field in read and write operations? You
    defined all three values but you are only using 'flush' here.

  - For the case of flush you could set sectors-count to the total size
    of the BlockDriverState as Eric suggested (bdrv_nb_sectors(bs)).
    Setting both to 0 could confuse clients that are not ready to deal
    with flush failures.

  - Since the QuorumAIOCB parameter is not used in the flush case, you
    could replace it from the function prototype and use sector_num and
    nb_sectors instead. That way you can also omit the switch.

Berto

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

[Alberto Garcia]

On Wed 24 Feb 2016 11:11:55 AM CET, Changlong Xie wrote:
> +        if (result) {
> +            quorum_report_bad(QUORUM_OP_TYPE_FLUSH, NULL,
> +                              s->children[i]->bs->node_name, result);

This line can change depending on what happens with patch 2, but the
rest looks correct to me, therefore:

Reviewed-by: Alberto Garcia <berto@igalia.com>

You can keep the Reviewed-by line as long as you only change the line
above to match the prototype set in patch 2.

Berto

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

[Eric Blake]

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

On 02/24/2016 05:35 AM, Alberto Garcia wrote:

>> +    switch (type) {
>> +    case QUORUM_OP_TYPE_READ:
>> +    case QUORUM_OP_TYPE_WRITE:
>> +        qapi_event_send_quorum_report_bad(false, 0, !!msg, msg, node_name,
>> +                                          acb->sector_num, acb->nb_sectors,
>> +                                          &error_abort);
>> +        break;
>> +    case QUORUM_OP_TYPE_FLUSH:
>> +        qapi_event_send_quorum_report_bad(true, type, !!msg, msg, node_name,
>> +                                          0, 0, &error_abort);
>> +        break;
> 
> A few comments:
> 
>   - Why don't you set the 'type' field in read and write operations? You
>     defined all three values but you are only using 'flush' here.

In fact, 'type' does not need to be optional; always outputting it makes
more sense for new clients, and doesn't hurt old clients.

-- 
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 v5 2/3] qmp event: Refactor QUORUM_REPORT_BAD

[Eric Blake]

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

On 02/24/2016 03:11 AM, Changlong Xie wrote:
> 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>
> ---

> +++ b/docs/qmp-events.txt
> @@ -307,6 +307,7 @@ Emitted to report a corruption of a Quorum file.
>  
>  Data:
>  
> +- "type":          Quorum operation type (json-string, optional)

I don't think 'type' needs to be optional, after all.  Just always
output it.

>  - "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
> @@ -318,10 +319,17 @@ Data:
>  
>  Example:
>  
> +Read/Write operation:
>  { "event": "QUORUM_REPORT_BAD",
>       "data": { "node-name": "node0", "sector-num": 345435, "sectors-count": 5 },
>       "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }

and this example would then show "type":"read"

-- 
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 v5 2/3] qmp event: Refactor QUORUM_REPORT_BAD

[Wen Congyang]

On 02/25/2016 12:59 AM, Eric Blake wrote:
> On 02/24/2016 03:11 AM, Changlong Xie wrote:
>> 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>
>> ---
> 
>> +++ b/docs/qmp-events.txt
>> @@ -307,6 +307,7 @@ Emitted to report a corruption of a Quorum file.
>>  
>>  Data:
>>  
>> +- "type":          Quorum operation type (json-string, optional)
> 
> I don't think 'type' needs to be optional, after all.  Just always
> output it.

If we output read/write type, old libvirt will ignore the read/write error events?

Thanks
Wen Congyang

> 
>>  - "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
>> @@ -318,10 +319,17 @@ Data:
>>  
>>  Example:
>>  
>> +Read/Write operation:
>>  { "event": "QUORUM_REPORT_BAD",
>>       "data": { "node-name": "node0", "sector-num": 345435, "sectors-count": 5 },
>>       "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
> 
> and this example would then show "type":"read"
> 

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

[Eric Blake]

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

On 02/24/2016 05:50 PM, Wen Congyang wrote:

>>> +- "type":          Quorum operation type (json-string, optional)
>>
>> I don't think 'type' needs to be optional, after all.  Just always
>> output it.
> 
> If we output read/write type, old libvirt will ignore the read/write error events?

The QMP protocol already documents that ALL clients MUST ignore
unexpected output fields.  Any client that is unprepared for new fields
appearing in the dictionary is buggy.  Libvirt will be just fine if you
output a new "type":"read".

-- 
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 v5 2/3] qmp event: Refactor QUORUM_REPORT_BAD

[Changlong Xie]

On 02/25/2016 09:12 AM, Eric Blake wrote:
> On 02/24/2016 05:50 PM, Wen Congyang wrote:
>
>>>> +- "type":          Quorum operation type (json-string, optional)
>>>
>>> I don't think 'type' needs to be optional, after all.  Just always
>>> output it.
>>
>> If we output read/write type, old libvirt will ignore the read/write error events?
>
> The QMP protocol already documents that ALL clients MUST ignore
> unexpected output fields.  Any client that is unprepared for new fields
> appearing in the dictionary is buggy.  Libvirt will be just fine if you
> output a new "type":"read".

Ok, i'll make "type" mandatory.

Thanks
	-Xie

>

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

[Changlong Xie]

On 02/24/2016 08:35 PM, Alberto Garcia wrote:
> On Wed 24 Feb 2016 11:11:54 AM CET, Changlong Xie wrote:
>> -static void quorum_report_bad(QuorumAIOCB *acb, char *node_name, int ret)
>> +static void quorum_report_bad(QuorumOpType type, QuorumAIOCB *acb,
>> +                              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);
>> +
>> +    switch (type) {
>> +    case QUORUM_OP_TYPE_READ:
>> +    case QUORUM_OP_TYPE_WRITE:
>> +        qapi_event_send_quorum_report_bad(false, 0, !!msg, msg, node_name,
>> +                                          acb->sector_num, acb->nb_sectors,
>> +                                          &error_abort);
>> +        break;
>> +    case QUORUM_OP_TYPE_FLUSH:
>> +        qapi_event_send_quorum_report_bad(true, type, !!msg, msg, node_name,
>> +                                          0, 0, &error_abort);
>> +        break;
>
> A few comments:
>
>    - Why don't you set the 'type' field in read and write operations? You
>      defined all three values but you are only using 'flush' here.
>
>    - For the case of flush you could set sectors-count to the total size
>      of the BlockDriverState as Eric suggested (bdrv_nb_sectors(bs)).
>      Setting both to 0 could confuse clients that are not ready to deal
>      with flush failures.
>
>    - Since the QuorumAIOCB parameter is not used in the flush case, you
>      could replace it from the function prototype and use sector_num and
>      nb_sectors instead. That way you can also omit the switch.
>

Surely, all your suggestions are helpful. Also i will add "Reviewed-by" 
in patch 1/3, 3/3 in next version.

Thanks
	-Xie

> Berto
>
>
> .
>