diff mbox

[v5,2/3] qmp event: Refactor QUORUM_REPORT_BAD

Message ID 1456308715-15465-3-git-send-email-xiecl.fnst@cn.fujitsu.com (mailing list archive)
State New, archived
Headers show

Commit Message

Changlong Xie Feb. 24, 2016, 10:11 a.m. UTC 
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(-)

Comments

Alberto Garcia Feb. 24, 2016, 12:35 p.m. UTC | #1 
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
Eric Blake Feb. 24, 2016, 4:57 p.m. UTC | #2 
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 Feb. 24, 2016, 4:59 p.m. UTC | #3 
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"
Wen Congyang Feb. 25, 2016, 12:50 a.m. UTC | #4 
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"
>
Eric Blake Feb. 25, 2016, 1:12 a.m. UTC | #5 
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".
Changlong Xie Feb. 25, 2016, 1:24 a.m. UTC | #6 
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

>
Changlong Xie Feb. 25, 2016, 2:44 a.m. UTC | #7 
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
>
>
> .
>
diff mbox

Patch

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' } }
 
 ##