* [Qemu-devel] [PATCH v4 0/2] modify vote rules for flush operation
@ 2016-02-23 9:01 Changlong Xie
2016-02-23 9:01 ` [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR Changlong Xie
2016-02-23 9:01 ` [Qemu-devel] [PATCH v4 2/2] quorum: modify vote rules for flush operation Changlong Xie
0 siblings, 2 replies; 11+ messages in thread
From: Changlong Xie @ 2016-02-23 9:01 UTC (permalink / raw)
To: qemu devel, Alberto Garcia, Eric Blake, Kevin Wolf, Max Reitz
Cc: Dr. David Alan Gilbert
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR
2016-02-23 9:01 [Qemu-devel] [PATCH v4 0/2] modify vote rules for flush operation Changlong Xie
@ 2016-02-23 9:01 ` Changlong Xie
2016-02-23 9:07 ` Changlong Xie
` (2 more replies)
2016-02-23 9:01 ` [Qemu-devel] [PATCH v4 2/2] quorum: modify vote rules for flush operation Changlong Xie
1 sibling, 3 replies; 11+ messages in thread
From: Changlong Xie @ 2016-02-23 9:01 UTC (permalink / raw)
To: qemu devel, Alberto Garcia, Eric Blake, Kevin Wolf, Max Reitz
Cc: Dr. David Alan Gilbert
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v4 2/2] quorum: modify vote rules for flush operation
2016-02-23 9:01 [Qemu-devel] [PATCH v4 0/2] modify vote rules for flush operation Changlong Xie
2016-02-23 9:01 ` [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR Changlong Xie
@ 2016-02-23 9:01 ` Changlong Xie
2016-02-23 12:56 ` Alberto Garcia
1 sibling, 1 reply; 11+ messages in thread
From: Changlong Xie @ 2016-02-23 9:01 UTC (permalink / raw)
To: qemu devel, Alberto Garcia, Eric Blake, Kevin Wolf, Max Reitz
Cc: Dr. David Alan Gilbert
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR
2016-02-23 9:01 ` [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR Changlong Xie
@ 2016-02-23 9:07 ` Changlong Xie
2016-02-23 12:53 ` Alberto Garcia
2016-02-23 13:17 ` Eric Blake
2 siblings, 0 replies; 11+ messages in thread
From: Changlong Xie @ 2016-02-23 9:07 UTC (permalink / raw)
To: qemu devel, Alberto Garcia, Eric Blake, Kevin Wolf, Max Reitz,
Markus Armbruster
Cc: Dr. David Alan Gilbert
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.
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR
2016-02-23 9:01 ` [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR Changlong Xie
2016-02-23 9:07 ` Changlong Xie
@ 2016-02-23 12:53 ` Alberto Garcia
2016-02-23 13:17 ` Eric Blake
2 siblings, 0 replies; 11+ messages in thread
From: Alberto Garcia @ 2016-02-23 12:53 UTC (permalink / raw)
To: Changlong Xie, qemu devel, Eric Blake, Kevin Wolf, Max Reitz
Cc: Dr. David Alan Gilbert
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] quorum: modify vote rules for flush operation
2016-02-23 9:01 ` [Qemu-devel] [PATCH v4 2/2] quorum: modify vote rules for flush operation Changlong Xie
@ 2016-02-23 12:56 ` Alberto Garcia
0 siblings, 0 replies; 11+ messages in thread
From: Alberto Garcia @ 2016-02-23 12:56 UTC (permalink / raw)
To: Changlong Xie, qemu devel, Eric Blake, Kevin Wolf, Max Reitz
Cc: Dr. David Alan Gilbert
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR
2016-02-23 9:01 ` [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR Changlong Xie
2016-02-23 9:07 ` Changlong Xie
2016-02-23 12:53 ` Alberto Garcia
@ 2016-02-23 13:17 ` Eric Blake
2016-02-23 13:24 ` Alberto Garcia
2 siblings, 1 reply; 11+ messages in thread
From: Eric Blake @ 2016-02-23 13:17 UTC (permalink / raw)
To: Changlong Xie, qemu devel, Alberto Garcia, Kevin Wolf, Max Reitz
Cc: Dr. David Alan Gilbert
[-- 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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR
2016-02-23 13:17 ` Eric Blake
@ 2016-02-23 13:24 ` Alberto Garcia
2016-02-23 13:45 ` Eric Blake
0 siblings, 1 reply; 11+ messages in thread
From: Alberto Garcia @ 2016-02-23 13:24 UTC (permalink / raw)
To: Eric Blake, Changlong Xie, qemu devel, Kevin Wolf, Max Reitz
Cc: Dr. David Alan Gilbert
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR
2016-02-23 13:24 ` Alberto Garcia
@ 2016-02-23 13:45 ` Eric Blake
2016-02-23 14:05 ` Alberto Garcia
0 siblings, 1 reply; 11+ messages in thread
From: Eric Blake @ 2016-02-23 13:45 UTC (permalink / raw)
To: Alberto Garcia, Changlong Xie, qemu devel, Kevin Wolf, Max Reitz
Cc: Dr. David Alan Gilbert
[-- 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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR
2016-02-23 13:45 ` Eric Blake
@ 2016-02-23 14:05 ` Alberto Garcia
2016-02-24 2:49 ` Changlong Xie
0 siblings, 1 reply; 11+ messages in thread
From: Alberto Garcia @ 2016-02-23 14:05 UTC (permalink / raw)
To: Eric Blake, Changlong Xie, qemu devel, Kevin Wolf, Max Reitz
Cc: Dr. David Alan Gilbert
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR
2016-02-23 14:05 ` Alberto Garcia
@ 2016-02-24 2:49 ` Changlong Xie
0 siblings, 0 replies; 11+ messages in thread
From: Changlong Xie @ 2016-02-24 2:49 UTC (permalink / raw)
To: Alberto Garcia, Eric Blake, qemu devel, Kevin Wolf, Max Reitz
Cc: Dr. David Alan Gilbert
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
>
>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-02-24 2:48 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-23 9:01 [Qemu-devel] [PATCH v4 0/2] modify vote rules for flush operation Changlong Xie
2016-02-23 9:01 ` [Qemu-devel] [PATCH v4 1/2] qmp event: Add QUORUM_FLUSH_ERROR Changlong Xie
2016-02-23 9:07 ` Changlong Xie
2016-02-23 12:53 ` Alberto Garcia
2016-02-23 13:17 ` Eric Blake
2016-02-23 13:24 ` Alberto Garcia
2016-02-23 13:45 ` Eric Blake
2016-02-23 14:05 ` Alberto Garcia
2016-02-24 2:49 ` Changlong Xie
2016-02-23 9:01 ` [Qemu-devel] [PATCH v4 2/2] quorum: modify vote rules for flush operation Changlong Xie
2016-02-23 12:56 ` Alberto Garcia
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.