[Qemu-devel] [PATCH v4 0/2] modify vote rules for flush operation

[Qemu-devel] [PATCH v4 0/2] modify vote rules for flush operation

[Changlong Xie]

ChangLog:
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 (2):
  qmp event: Add QUORUM_FLUSH_ERROR
  quorum: modify vote rules for flush operation

 block/quorum.c      | 24 +++++++++++++++++++-----
 docs/qmp-events.txt | 18 ++++++++++++++++++
 qapi/event.json     | 16 ++++++++++++++++
 3 files changed, 53 insertions(+), 5 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR

[Changlong Xie]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
---
 block/quorum.c      |  5 +++++
 docs/qmp-events.txt | 18 ++++++++++++++++++
 qapi/event.json     | 16 ++++++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/block/quorum.c b/block/quorum.c
index f78d4cb..d3c3958 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -235,6 +235,11 @@ static void quorum_report_failure(QuorumAIOCB *acb)
                                    acb->nb_sectors, &error_abort);
 }
 
+static void quorum_flush_error(char *node_name, const char *msg)
+{
+    qapi_event_send_quorum_flush_error(node_name, msg, &error_abort);
+}
+
 static int quorum_vote_error(QuorumAIOCB *acb);
 
 static bool quorum_has_too_much_io_failed(QuorumAIOCB *acb)
diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index b6e8937..d777873 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -340,6 +340,24 @@ Example:
 
 Note: this event is rate-limited.
 
+QUORUM_FLUSH_ERROR
+-----------------
+
+Emitted to report flush error message of the Quorum block driver
+
+Data:
+
+- "node-name":     The graph node name of the block driver state.
+- "error":         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 try to interpret the error string.
+
+Example:
+
+{ "event": "QUORUM_FLUSH_ERROR",
+     "data": { "node-name": "1.raw", "error": "xxxxxx" },
+     "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
+
 RESET
 -----
 
diff --git a/qapi/event.json b/qapi/event.json
index cfcc887..5b16706 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -358,6 +358,22 @@
             'sector-num': 'int', 'sectors-count': 'int' } }
 
 ##
+# @QUORUM_FLUSH_ERROR
+#
+# Emitted to report flush error message of the Quorum block driver
+#
+# @node-name: the graph node name of the block driver state
+#
+# @error: 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
+#         try to interpret the error string.
+#
+# Since: 2.5
+##
+{ 'event': 'QUORUM_FLUSH_ERROR',
+  'data': { 'node-name': 'str', 'error': 'str'} }
+
+##
 # @VSERPORT_CHANGE
 #
 # Emitted when the guest opens or closes a virtio-serial port.
-- 
1.9.3

Re: [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR

[Changlong Xie]

cc: Markus Armbruster <armbru@redhat.com>

On 02/23/2016 05:01 PM, Changlong Xie wrote:
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> ---
>   block/quorum.c      |  5 +++++
>   docs/qmp-events.txt | 18 ++++++++++++++++++
>   qapi/event.json     | 16 ++++++++++++++++
>   3 files changed, 39 insertions(+)
>
> diff --git a/block/quorum.c b/block/quorum.c
> index f78d4cb..d3c3958 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -235,6 +235,11 @@ static void quorum_report_failure(QuorumAIOCB *acb)
>                                      acb->nb_sectors, &error_abort);
>   }
>
> +static void quorum_flush_error(char *node_name, const char *msg)
> +{
> +    qapi_event_send_quorum_flush_error(node_name, msg, &error_abort);
> +}
> +
>   static int quorum_vote_error(QuorumAIOCB *acb);
>
>   static bool quorum_has_too_much_io_failed(QuorumAIOCB *acb)
> diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
> index b6e8937..d777873 100644
> --- a/docs/qmp-events.txt
> +++ b/docs/qmp-events.txt
> @@ -340,6 +340,24 @@ Example:
>
>   Note: this event is rate-limited.
>
> +QUORUM_FLUSH_ERROR
> +-----------------
> +
> +Emitted to report flush error message of the Quorum block driver
> +
> +Data:
> +
> +- "node-name":     The graph node name of the block driver state.
> +- "error":         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 try to interpret the error string.
> +
> +Example:
> +
> +{ "event": "QUORUM_FLUSH_ERROR",
> +     "data": { "node-name": "1.raw", "error": "xxxxxx" },
> +     "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
> +
>   RESET
>   -----
>
> diff --git a/qapi/event.json b/qapi/event.json
> index cfcc887..5b16706 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -358,6 +358,22 @@
>               'sector-num': 'int', 'sectors-count': 'int' } }
>
>   ##
> +# @QUORUM_FLUSH_ERROR
> +#
> +# Emitted to report flush error message of the Quorum block driver
> +#
> +# @node-name: the graph node name of the block driver state
> +#
> +# @error: 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
> +#         try to interpret the error string.
> +#
> +# Since: 2.5
> +##
> +{ 'event': 'QUORUM_FLUSH_ERROR',
> +  'data': { 'node-name': 'str', 'error': 'str'} }
> +
> +##
>   # @VSERPORT_CHANGE
>   #
>   # Emitted when the guest opens or closes a virtio-serial port.
>

Re: [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR

[Alberto Garcia]

On Tue 23 Feb 2016 10:01:38 AM CET, Changlong Xie wrote:

> @@ -235,6 +235,11 @@ static void quorum_report_failure(QuorumAIOCB *acb)
>                                     acb->nb_sectors, &error_abort);
>  }
>  
> +static void quorum_flush_error(char *node_name, const char *msg)
> +{
> +    qapi_event_send_quorum_flush_error(node_name, msg, &error_abort);
> +}
> +

Instead of 'msg' I think you can receive an error code here and convert
it to a message using strerror(). Take a look at quorum_report_bad() to
see what I mean.

> +QUORUM_FLUSH_ERROR
> +-----------------
> +
> +Emitted to report flush error message of the Quorum block driver

Emitted to report a flush error in a Quorum file

> +
> +Data:
> +
> +- "node-name":     The graph node name of the block driver state.
> +- "error":         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 try to interpret the error string.

Can you reformat this paragraph so it takes less than 80 columns?

> +
> +Example:
> +
> +{ "event": "QUORUM_FLUSH_ERROR",
> +     "data": { "node-name": "1.raw", "error": "xxxxxx" },
> +     "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }

'1.raw' is an invalid node name.

>  ##
> +# @QUORUM_FLUSH_ERROR
> +#
> +# Emitted to report flush error message of the Quorum block driver
> +#
> +# @node-name: the graph node name of the block driver state
> +#
> +# @error: 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
> +#         try to interpret the error string.

Same changes I mentioned earlier.

> +#
> +# Since: 2.5

Since 2.6

Berto

Re: [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR

[Eric Blake]

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

On 02/23/2016 02:01 AM, Changlong Xie wrote:
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> ---
>  block/quorum.c      |  5 +++++
>  docs/qmp-events.txt | 18 ++++++++++++++++++
>  qapi/event.json     | 16 ++++++++++++++++
>  3 files changed, 39 insertions(+)

In addition to Berto's findings:


> +++ b/docs/qmp-events.txt
> @@ -340,6 +340,24 @@ Example:
>  
>  Note: this event is rate-limited.
>  
> +QUORUM_FLUSH_ERROR
> +-----------------

Please keep the file sorted; this would fall between QUORUM_FAILURE and
QUORUM_REPORT_BAD.  Commit message should say why we need a third event,
rather than reusing either of the other two (my guess: because you don't
have a location, and don't want to modify the existing two to report a
location - but why not just use 'sector-num':0, 'sectors-count':<size of
file> to report the entire file as the location?)

Length of ---- separator should match text above it (you were off by one).

Is this event rate-limited? Should it be?

-- 
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 v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR

[Alberto Garcia]

On Tue 23 Feb 2016 02:17:23 PM CET, Eric Blake wrote:

> Commit message should say why we need a third event, rather than
> reusing either of the other two (my guess: because you don't have a
> location, and don't want to modify the existing two to report a
> location - but why not just use 'sector-num':0, 'sectors-count':<size
> of file> to report the entire file as the location?)

I would also be fine with that solution.

Berto

Re: [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR

[Eric Blake]

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

On 02/23/2016 06:24 AM, Alberto Garcia wrote:
> On Tue 23 Feb 2016 02:17:23 PM CET, Eric Blake wrote:
> 
>> Commit message should say why we need a third event, rather than
>> reusing either of the other two (my guess: because you don't have a
>> location, and don't want to modify the existing two to report a
>> location - but why not just use 'sector-num':0, 'sectors-count':<size
>> of file> to report the entire file as the location?)
> 
> I would also be fine with that solution.

I would also be fine if we added an optional enum member to the existing
event that said which operation failed ('read', 'write', 'flush') -
adding optional output members is safe, while converting existing
mandatory output members to optional may confuse existing 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 v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR

[Alberto Garcia]

On Tue 23 Feb 2016 02:45:49 PM CET, Eric Blake wrote:

>>> Commit message should say why we need a third event, rather than
>>> reusing either of the other two (my guess: because you don't have a
>>> location, and don't want to modify the existing two to report a
>>> location - but why not just use 'sector-num':0,
>>> 'sectors-count':<size of file> to report the entire file as the
>>> location?)
>> 
>> I would also be fine with that solution.
>
> I would also be fine if we added an optional enum member to the
> existing event that said which operation failed ('read', 'write',
> 'flush') - adding optional output members is safe, while converting
> existing mandatory output members to optional may confuse existing
> clients.

That might actually be the best option :-)

Berto

Re: [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR

[Changlong Xie]

On 02/23/2016 10:05 PM, Alberto Garcia wrote:
> On Tue 23 Feb 2016 02:45:49 PM CET, Eric Blake wrote:
>
>>>> Commit message should say why we need a third event, rather than
>>>> reusing either of the other two (my guess: because you don't have a
>>>> location, and don't want to modify the existing two to report a
>>>> location - but why not just use 'sector-num':0,
>>>> 'sectors-count':<size of file> to report the entire file as the
>>>> location?)
>>>
>>> I would also be fine with that solution.
>>
>> I would also be fine if we added an optional enum member to the
>> existing event that said which operation failed ('read', 'write',
>> 'flush') - adding optional output members is safe, while converting
>> existing mandatory output members to optional may confuse existing
>> clients.
>

Hi Berto & Eric

Thanks for all your comments. Surely, this is the best option to me too
:-), will fix it in next version.

Thanks
	-Xie

> That might actually be the best option :-)
>
> Berto
>
>
> .
>

[Qemu-devel] [PATCH v4 2/2] 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 | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index d3c3958..6048208 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -768,19 +768,28 @@ 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_flush_error(s->children[i]->bs->node_name, "Flush failed");
+            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 v4 2/2] quorum: modify vote rules for flush operation

[Alberto Garcia]

On Tue 23 Feb 2016 10:01:39 AM CET, Changlong Xie wrote:
>      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_flush_error(s->children[i]->bs->node_name, "Flush failed");
> +            result_value.l = result;
> +            quorum_count_vote(&error_votes, &result_value, i);
> +        } else {
> +            success_count++;
> +        }

As I wrote on the review of patch 1/2, I would pass 'result' directly
instead of "Flush failed". Otherwise this patch looks good, thanks!

Berto