[PATCH 0/7] oxenstored: improve transaction conflict handling

[PATCH 0/7] oxenstored: improve transaction conflict handling

[Jonathan Davies]

This patch series makes a substantial improvement to oxenstored's transaction
handling.

The original design of oxenstored assumed that a transaction would only ever
span a small subtree. In practice this is rarely the case and means that other
xenstore traffic overlapping with a transaction can cause it to fail with
EAGAIN. This leads to significant performance problems with toolstack operations
when there is a certain level of xenstore traffic coming from other sources.

Instead, we observe that, although the transaction may span a large subtree, it
is very unlikely to read or write the same keys as other xenstore traffic. This
observation leads to an improved transaction conflict-handling algorithm in
which the transaction is replayed to check it is serializable. This approach is
superior in performance.

The sixth patch is the main one that changes the way transaction conflicts are
handled. The first five patches prepare the way by performing some refactoring
and addition of infrastructure that allows for transactions to be replayed. The
seventh patch adds some extra logging.

Jonathan Davies (7):
  oxenstored: refactor putting response on wire
  oxenstored: remove some unused parameters
  oxenstored: refactor request processing
  oxenstored: keep track of each transaction's operations
  oxenstored: move functions that process simple operations
  oxenstored: replay transaction upon conflict
  oxenstored: log request and response during transaction replay

 tools/ocaml/xenstored/Makefile        |    1 +
 tools/ocaml/xenstored/connection.ml   |    5 +-
 tools/ocaml/xenstored/define.ml       |    1 +
 tools/ocaml/xenstored/oxenstored.conf |    1 +
 tools/ocaml/xenstored/process.ml      |  344 +++++++++++++++++++++------------
 tools/ocaml/xenstored/transaction.ml  |   21 +-
 tools/ocaml/xenstored/xenstored.ml    |    1 +
 7 files changed, 239 insertions(+), 135 deletions(-)

-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 1/7] oxenstored: refactor putting response on wire

[Jonathan Davies]

Previously, the functions reply_{ack,data,data_or_ack} and input_handle_error
put the response on the wire by invoking Connection.send_{ack,reply,error}.

Instead, these functions now return a value indicating what needs to be put on
the wire, and that action is done by a send_response function called
afterwards.

This refactoring gives us a chance to store the value of the response, useful
for replaying transactions.

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Reviewed-by: Euan Harris <euan.harris@citrix.com>
---
 tools/ocaml/xenstored/Makefile   |    1 +
 tools/ocaml/xenstored/process.ml |   34 ++++++++++++++++++++++++----------
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/tools/ocaml/xenstored/Makefile b/tools/ocaml/xenstored/Makefile
index 59875f7..dce9e70 100644
--- a/tools/ocaml/xenstored/Makefile
+++ b/tools/ocaml/xenstored/Makefile
@@ -36,6 +36,7 @@ OBJS = define \
 	stdext \
 	trie \
 	config \
+	packet \
 	logging \
 	quota \
 	perms \
diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index e827678..3377966 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -126,8 +126,7 @@ let do_watch con t rid domains cons data =
 		| _                   -> raise Invalid_Cmd_Args
 		in
 	let watch = Connections.add_watch cons con node token in
-	Connection.send_ack con (Transaction.get_id t) rid Xenbus.Xb.Op.Watch;
-	Connection.fire_single_watch watch
+	Packet.Ack (fun () -> Connection.fire_single_watch watch)
 
 let do_unwatch con t domains cons data =
 	let (node, token) =
@@ -289,20 +288,32 @@ let do_set_target con t domains cons data =
 		| _                           -> raise Invalid_Cmd_Args
 
 (*------------- Generic handling of ty ------------------*)
+let send_response ty con t rid response =
+	match response with
+	| Packet.Ack f ->
+		Connection.send_ack con (Transaction.get_id t) rid ty;
+		(* Now do any necessary follow-up actions *)
+		f ()
+	| Packet.Reply ret ->
+		Connection.send_reply con (Transaction.get_id t) rid ty ret
+	| Packet.Error e ->
+		Connection.send_error con (Transaction.get_id t) rid e
+
 let reply_ack fct ty con t rid doms cons data =
 	fct con t doms cons data;
-	Connection.send_ack con (Transaction.get_id t) rid ty;
-	if Transaction.get_id t = Transaction.none then
-		process_watch (Transaction.get_ops t) cons
+	Packet.Ack (fun () ->
+		if Transaction.get_id t = Transaction.none then
+			process_watch (Transaction.get_ops t) cons
+	)
 
 let reply_data fct ty con t rid doms cons data =
 	let ret = fct con t doms cons data in
-	Connection.send_reply con (Transaction.get_id t) rid ty ret
+	Packet.Reply ret
 
 let reply_data_or_ack fct ty con t rid doms cons data =
 	match fct con t doms cons data with
-		| Some ret -> Connection.send_reply con (Transaction.get_id t) rid ty ret
-		| None -> Connection.send_ack con (Transaction.get_id t) rid ty
+		| Some ret -> Packet.Reply ret
+		| None -> Packet.Ack (fun () -> ())
 
 let reply_none fct ty con t rid doms cons data =
 	(* let the function reply *)
@@ -335,7 +346,7 @@ let function_of_type ty =
 
 let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
 	let reply_error e =
-		Connection.send_error con (Transaction.get_id t) rid e in
+		Packet.Error e in
 	try
 		fct ty con t rid doms cons data
 	with
@@ -368,7 +379,10 @@ let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
 			else
 				Connection.get_transaction con tid
 			in
-		input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data;
+		let response = input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data in
+
+		(* Put the response on the wire *)
+		send_response ty con t rid response
 	with exn ->
 		error "process packet: %s" (Printexc.to_string exn);
 		Connection.send_error con tid rid "EIO"
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 2/7] oxenstored: remove some unused parameters

[Jonathan Davies]

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Reviewed-by: Euan Harris <euan.harris@citrix.com>
---
 tools/ocaml/xenstored/process.ml |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index 3377966..7a73669 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -119,7 +119,7 @@ let do_getperms con t domains cons data =
 	let perms = Transaction.getperms t (Connection.get_perm con) path in
 	Perms.Node.to_string perms ^ "\000"
 
-let do_watch con t rid domains cons data =
+let do_watch con t domains cons data =
 	let (node, token) = 
 		match (split None '\000' data) with
 		| [node; token; ""]   -> node, token
@@ -299,25 +299,25 @@ let send_response ty con t rid response =
 	| Packet.Error e ->
 		Connection.send_error con (Transaction.get_id t) rid e
 
-let reply_ack fct ty con t rid doms cons data =
+let reply_ack fct con t doms cons data =
 	fct con t doms cons data;
 	Packet.Ack (fun () ->
 		if Transaction.get_id t = Transaction.none then
 			process_watch (Transaction.get_ops t) cons
 	)
 
-let reply_data fct ty con t rid doms cons data =
+let reply_data fct con t doms cons data =
 	let ret = fct con t doms cons data in
 	Packet.Reply ret
 
-let reply_data_or_ack fct ty con t rid doms cons data =
+let reply_data_or_ack fct con t doms cons data =
 	match fct con t doms cons data with
 		| Some ret -> Packet.Reply ret
 		| None -> Packet.Ack (fun () -> ())
 
-let reply_none fct ty con t rid doms cons data =
+let reply_none fct con t doms cons data =
 	(* let the function reply *)
-	fct con t rid doms cons data
+	fct con t doms cons data
 
 let function_of_type ty =
 	match ty with
@@ -348,7 +348,7 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
 	let reply_error e =
 		Packet.Error e in
 	try
-		fct ty con t rid doms cons data
+		fct con t doms cons data
 	with
 	| Define.Invalid_path          -> reply_error "EINVAL"
 	| Define.Already_exist         -> reply_error "EEXIST"
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 3/7] oxenstored: refactor request processing

[Jonathan Davies]

Encapsulate the request in a record that is passed from do_input to
process_packet and input_handle_error.

This will be helpful when keeping track of the requests made as part of a
transaction.

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Reviewed-by: Euan Harris <euan.harris@citrix.com>
---
 tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index 7a73669..c92bec7 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -344,11 +344,11 @@ let function_of_type ty =
 	| Xenbus.Xb.Op.Invalid           -> reply_ack do_error
 	| _                              -> reply_ack do_error
 
-let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
+let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
 	let reply_error e =
 		Packet.Error e in
 	try
-		fct con t doms cons data
+		fct con t doms cons req.Packet.data
 	with
 	| Define.Invalid_path          -> reply_error "EINVAL"
 	| Define.Already_exist         -> reply_error "EEXIST"
@@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
 (**
  * Nothrow guarantee.
  *)
-let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
+let process_packet ~store ~cons ~doms ~con ~req =
+	let ty = req.Packet.ty in
+	let tid = req.Packet.tid in
+	let rid = req.Packet.rid in
 	try
 		let fct = function_of_type ty in
 		let t =
@@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
 			else
 				Connection.get_transaction con tid
 			in
-		let response = input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data in
+		let response = input_handle_error ~cons ~doms ~fct ~con ~t ~req in
 
 		(* Put the response on the wire *)
 		send_response ty con t rid response
@@ -412,11 +415,13 @@ let do_input store cons doms con =
 	if newpacket then (
 		let packet = Connection.pop_in con in
 		let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
+		let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
+
 		(* As we don't log IO, do not call an unnecessary sanitize_data 
 		   info "[%s] -> [%d] %s \"%s\""
 		         (Connection.get_domstr con) tid
 		         (Xenbus.Xb.Op.to_string ty) (sanitize_data data); *)
-		process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data;
+		process_packet ~store ~cons ~doms ~con ~req;
 		write_access_log ~ty ~tid ~con ~data;
 		Connection.incr_ops con;
 	)
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/7] oxenstored: refactor request processing

[Boris Ostrovsky]

On 03/17/2016 01:51 PM, Jonathan Davies wrote:
> Encapsulate the request in a record that is passed from do_input to
> process_packet and input_handle_error.
>
> This will be helpful when keeping track of the requests made as part of a
> transaction.
>
> Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
> Reviewed-by: Euan Harris <euan.harris@citrix.com>
> ---
>   tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
> index 7a73669..c92bec7 100644
> --- a/tools/ocaml/xenstored/process.ml
> +++ b/tools/ocaml/xenstored/process.ml
> @@ -344,11 +344,11 @@ let function_of_type ty =
>   	| Xenbus.Xb.Op.Invalid           -> reply_ack do_error
>   	| _                              -> reply_ack do_error
>   
> -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
> +let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
>   	let reply_error e =
>   		Packet.Error e in
>   	try
> -		fct con t doms cons data
> +		fct con t doms cons req.Packet.data
>   	with
>   	| Define.Invalid_path          -> reply_error "EINVAL"
>   	| Define.Already_exist         -> reply_error "EEXIST"
> @@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
>   (**
>    * Nothrow guarantee.
>    *)
> -let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
> +let process_packet ~store ~cons ~doms ~con ~req =
> +	let ty = req.Packet.ty in
> +	let tid = req.Packet.tid in
> +	let rid = req.Packet.rid in
>   	try
>   		let fct = function_of_type ty in
>   		let t =
> @@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
>   			else
>   				Connection.get_transaction con tid
>   			in
> -		let response = input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data in
> +		let response = input_handle_error ~cons ~doms ~fct ~con ~t ~req in
>   
>   		(* Put the response on the wire *)
>   		send_response ty con t rid response
> @@ -412,11 +415,13 @@ let do_input store cons doms con =
>   	if newpacket then (
>   		let packet = Connection.pop_in con in
>   		let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
> +		let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
> +

I think this change breaks the build with older ocaml versions:

root@haswell> ocamlopt -v
The OCaml native-code compiler, version 4.00.1
Standard library directory: /usr/lib64/ocaml
root@haswell> ocamlopt -g -ccopt "    " -dtypes -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F 
-warn-error F -c -o process.cmx process.ml
root@haswell>


root@ovs104> ocamlopt -v
The Objective Caml native-code compiler, version 3.11.2
Standard library directory: /usr/lib64/ocaml
root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I 
/root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F 
-warn-error F -c -o process.cmx process.ml
File "process.ml", line 487, characters 23-24:
Error: Syntax error
root@ovs104>

I don't know much about ocaml (OK, I know *nothing* about ocaml) so I 
can't say what exactly might be wrong.

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/7] oxenstored: refactor request processing

[Andrew Cooper]

On 24/03/2016 22:22, Boris Ostrovsky wrote:
> On 03/17/2016 01:51 PM, Jonathan Davies wrote:
>> Encapsulate the request in a record that is passed from do_input to
>> process_packet and input_handle_error.
>>
>> This will be helpful when keeping track of the requests made as part
>> of a
>> transaction.
>>
>> Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
>> Reviewed-by: Euan Harris <euan.harris@citrix.com>
>> ---
>>   tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/ocaml/xenstored/process.ml
>> b/tools/ocaml/xenstored/process.ml
>> index 7a73669..c92bec7 100644
>> --- a/tools/ocaml/xenstored/process.ml
>> +++ b/tools/ocaml/xenstored/process.ml
>> @@ -344,11 +344,11 @@ let function_of_type ty =
>>       | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
>>       | _                              -> reply_ack do_error
>>   -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
>> +let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
>>       let reply_error e =
>>           Packet.Error e in
>>       try
>> -        fct con t doms cons data
>> +        fct con t doms cons req.Packet.data
>>       with
>>       | Define.Invalid_path          -> reply_error "EINVAL"
>>       | Define.Already_exist         -> reply_error "EEXIST"
>> @@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con
>> ~t ~rid ~data =
>>   (**
>>    * Nothrow guarantee.
>>    *)
>> -let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
>> +let process_packet ~store ~cons ~doms ~con ~req =
>> +    let ty = req.Packet.ty in
>> +    let tid = req.Packet.tid in
>> +    let rid = req.Packet.rid in
>>       try
>>           let fct = function_of_type ty in
>>           let t =
>> @@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid
>> ~rid ~ty ~data =
>>               else
>>                   Connection.get_transaction con tid
>>               in
>> -        let response = input_handle_error ~cons ~doms ~fct ~ty ~con
>> ~t ~rid ~data in
>> +        let response = input_handle_error ~cons ~doms ~fct ~con ~t
>> ~req in
>>             (* Put the response on the wire *)
>>           send_response ty con t rid response
>> @@ -412,11 +415,13 @@ let do_input store cons doms con =
>>       if newpacket then (
>>           let packet = Connection.pop_in con in
>>           let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
>> +        let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
>> +
>
> I think this change breaks the build with older ocaml versions:
>
> root@haswell> ocamlopt -v
> The OCaml native-code compiler, version 4.00.1
> Standard library directory: /usr/lib64/ocaml
> root@haswell> ocamlopt -g -ccopt "    " -dtypes -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> -warn-error F -c -o process.cmx process.ml
> root@haswell>
>
>
> root@ovs104> ocamlopt -v
> The Objective Caml native-code compiler, version 3.11.2
> Standard library directory: /usr/lib64/ocaml
> root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> -warn-error F -c -o process.cmx process.ml
> File "process.ml", line 487, characters 23-24:
> Error: Syntax error
> root@ovs104>
>
> I don't know much about ocaml (OK, I know *nothing* about ocaml) so I
> can't say what exactly might be wrong.

Could you perhaps try this instead?

let req = {tid = Packet.tid; rid = Packet.rid; ty = Packet.ty; data =
Packet.data} in

It is most likely that the older version of Ocaml can't infer the order
of fields.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/7] oxenstored: refactor request processing

[Boris Ostrovsky]

On 03/24/2016 06:49 PM, Andrew Cooper wrote:
> On 24/03/2016 22:22, Boris Ostrovsky wrote:
>> On 03/17/2016 01:51 PM, Jonathan Davies wrote:
>>> Encapsulate the request in a record that is passed from do_input to
>>> process_packet and input_handle_error.
>>>
>>> This will be helpful when keeping track of the requests made as part
>>> of a
>>> transaction.
>>>
>>> Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
>>> Reviewed-by: Euan Harris <euan.harris@citrix.com>
>>> ---
>>>    tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
>>>    1 file changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tools/ocaml/xenstored/process.ml
>>> b/tools/ocaml/xenstored/process.ml
>>> index 7a73669..c92bec7 100644
>>> --- a/tools/ocaml/xenstored/process.ml
>>> +++ b/tools/ocaml/xenstored/process.ml
>>> @@ -344,11 +344,11 @@ let function_of_type ty =
>>>        | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
>>>        | _                              -> reply_ack do_error
>>>    -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
>>> +let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
>>>        let reply_error e =
>>>            Packet.Error e in
>>>        try
>>> -        fct con t doms cons data
>>> +        fct con t doms cons req.Packet.data
>>>        with
>>>        | Define.Invalid_path          -> reply_error "EINVAL"
>>>        | Define.Already_exist         -> reply_error "EEXIST"
>>> @@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con
>>> ~t ~rid ~data =
>>>    (**
>>>     * Nothrow guarantee.
>>>     *)
>>> -let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
>>> +let process_packet ~store ~cons ~doms ~con ~req =
>>> +    let ty = req.Packet.ty in
>>> +    let tid = req.Packet.tid in
>>> +    let rid = req.Packet.rid in
>>>        try
>>>            let fct = function_of_type ty in
>>>            let t =
>>> @@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid
>>> ~rid ~ty ~data =
>>>                else
>>>                    Connection.get_transaction con tid
>>>                in
>>> -        let response = input_handle_error ~cons ~doms ~fct ~ty ~con
>>> ~t ~rid ~data in
>>> +        let response = input_handle_error ~cons ~doms ~fct ~con ~t
>>> ~req in
>>>              (* Put the response on the wire *)
>>>            send_response ty con t rid response
>>> @@ -412,11 +415,13 @@ let do_input store cons doms con =
>>>        if newpacket then (
>>>            let packet = Connection.pop_in con in
>>>            let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
>>> +        let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
>>> +
>> I think this change breaks the build with older ocaml versions:
>>
>> root@haswell> ocamlopt -v
>> The OCaml native-code compiler, version 4.00.1
>> Standard library directory: /usr/lib64/ocaml
>> root@haswell> ocamlopt -g -ccopt "    " -dtypes -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
>> -warn-error F -c -o process.cmx process.ml
>> root@haswell>
>>
>>
>> root@ovs104> ocamlopt -v
>> The Objective Caml native-code compiler, version 3.11.2
>> Standard library directory: /usr/lib64/ocaml
>> root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
>> -warn-error F -c -o process.cmx process.ml
>> File "process.ml", line 487, characters 23-24:
>> Error: Syntax error
>> root@ovs104>
>>
>> I don't know much about ocaml (OK, I know *nothing* about ocaml) so I
>> can't say what exactly might be wrong.
> Could you perhaps try this instead?
>
> let req = {tid = Packet.tid; rid = Packet.rid; ty = Packet.ty; data =
> Packet.data} in
>
> It is most likely that the older version of Ocaml can't infer the order
> of fields.
>
> ~Andrew


No, it now gives me "Error: Unbound record field label tid"

-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/7] oxenstored: refactor request processing

[Jonathan Davies]

On Thu, Mar 24, 2016 at 07:57:30PM -0400, Boris Ostrovsky wrote:
> On 03/24/2016 06:49 PM, Andrew Cooper wrote:
> >On 24/03/2016 22:22, Boris Ostrovsky wrote:
> >>On 03/17/2016 01:51 PM, Jonathan Davies wrote:
> >>>Encapsulate the request in a record that is passed from do_input to
> >>>process_packet and input_handle_error.
> >>>
> >>>This will be helpful when keeping track of the requests made as part
> >>>of a
> >>>transaction.
> >>>
> >>>Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
> >>>Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
> >>>Reviewed-by: Euan Harris <euan.harris@citrix.com>
> >>>---
> >>>   tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
> >>>   1 file changed, 10 insertions(+), 5 deletions(-)
> >>>
> >>>diff --git a/tools/ocaml/xenstored/process.ml
> >>>b/tools/ocaml/xenstored/process.ml
> >>>index 7a73669..c92bec7 100644
> >>>--- a/tools/ocaml/xenstored/process.ml
> >>>+++ b/tools/ocaml/xenstored/process.ml
> >>>@@ -344,11 +344,11 @@ let function_of_type ty =
> >>>       | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
> >>>       | _                              -> reply_ack do_error
> >>>   -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
> >>>+let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
> >>>       let reply_error e =
> >>>           Packet.Error e in
> >>>       try
> >>>-        fct con t doms cons data
> >>>+        fct con t doms cons req.Packet.data
> >>>       with
> >>>       | Define.Invalid_path          -> reply_error "EINVAL"
> >>>       | Define.Already_exist         -> reply_error "EEXIST"
> >>>@@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con
> >>>~t ~rid ~data =
> >>>   (**
> >>>    * Nothrow guarantee.
> >>>    *)
> >>>-let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
> >>>+let process_packet ~store ~cons ~doms ~con ~req =
> >>>+    let ty = req.Packet.ty in
> >>>+    let tid = req.Packet.tid in
> >>>+    let rid = req.Packet.rid in
> >>>       try
> >>>           let fct = function_of_type ty in
> >>>           let t =
> >>>@@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid
> >>>~rid ~ty ~data =
> >>>               else
> >>>                   Connection.get_transaction con tid
> >>>               in
> >>>-        let response = input_handle_error ~cons ~doms ~fct ~ty ~con
> >>>~t ~rid ~data in
> >>>+        let response = input_handle_error ~cons ~doms ~fct ~con ~t
> >>>~req in
> >>>             (* Put the response on the wire *)
> >>>           send_response ty con t rid response
> >>>@@ -412,11 +415,13 @@ let do_input store cons doms con =
> >>>       if newpacket then (
> >>>           let packet = Connection.pop_in con in
> >>>           let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
> >>>+        let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
> >>>+
> >>I think this change breaks the build with older ocaml versions:
> >>
> >>root@haswell> ocamlopt -v
> >>The OCaml native-code compiler, version 4.00.1
> >>Standard library directory: /usr/lib64/ocaml
> >>root@haswell> ocamlopt -g -ccopt "    " -dtypes -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> >>-warn-error F -c -o process.cmx process.ml
> >>root@haswell>
> >>
> >>
> >>root@ovs104> ocamlopt -v
> >>The Objective Caml native-code compiler, version 3.11.2
> >>Standard library directory: /usr/lib64/ocaml
> >>root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> >>/root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> >>-warn-error F -c -o process.cmx process.ml
> >>File "process.ml", line 487, characters 23-24:
> >>Error: Syntax error
> >>root@ovs104>
> >>
> >>I don't know much about ocaml (OK, I know *nothing* about ocaml) so I
> >>can't say what exactly might be wrong.
> >Could you perhaps try this instead?
> >
> >let req = {tid = Packet.tid; rid = Packet.rid; ty = Packet.ty; data =
> >Packet.data} in
> >
> >It is most likely that the older version of Ocaml can't infer the order
> >of fields.
> >
> >~Andrew
> 
> 
> No, it now gives me "Error: Unbound record field label tid"
> 
> -boris

Andrew's guess was close, but the wrong way around -- please could you try the
following with the older compiler?

let req = {Packet.tid=tid; Packet.rid=rid; Packet.ty=ty; Packet.data=data} in

I was using a syntactic feature of OCaml called 'field punning' which is
generally considered good practice and makes for more readable code. It looks
like this feature was introduced in OCaml 3.12.0 (dating from 2010), which is
consistent with Boris' findings.

What's the policy here -- is there a defined version of the OCaml compiler which
tools/ocaml needs to be able to compile with?

Thanks,
Jonathan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/7] oxenstored: refactor request processing

[Boris Ostrovsky]

On 03/29/2016 05:08 AM, Jonathan Davies wrote:
> On Thu, Mar 24, 2016 at 07:57:30PM -0400, Boris Ostrovsky wrote:
>> On 03/24/2016 06:49 PM, Andrew Cooper wrote:
>>> On 24/03/2016 22:22, Boris Ostrovsky wrote:
>>>> On 03/17/2016 01:51 PM, Jonathan Davies wrote:
>>>>> Encapsulate the request in a record that is passed from do_input to
>>>>> process_packet and input_handle_error.
>>>>>
>>>>> This will be helpful when keeping track of the requests made as part
>>>>> of a
>>>>> transaction.
>>>>>
>>>>> Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
>>>>> Reviewed-by: Euan Harris <euan.harris@citrix.com>
>>>>> ---
>>>>>    tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
>>>>>    1 file changed, 10 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/tools/ocaml/xenstored/process.ml
>>>>> b/tools/ocaml/xenstored/process.ml
>>>>> index 7a73669..c92bec7 100644
>>>>> --- a/tools/ocaml/xenstored/process.ml
>>>>> +++ b/tools/ocaml/xenstored/process.ml
>>>>> @@ -344,11 +344,11 @@ let function_of_type ty =
>>>>>        | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
>>>>>        | _                              -> reply_ack do_error
>>>>>    -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
>>>>> +let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
>>>>>        let reply_error e =
>>>>>            Packet.Error e in
>>>>>        try
>>>>> -        fct con t doms cons data
>>>>> +        fct con t doms cons req.Packet.data
>>>>>        with
>>>>>        | Define.Invalid_path          -> reply_error "EINVAL"
>>>>>        | Define.Already_exist         -> reply_error "EEXIST"
>>>>> @@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con
>>>>> ~t ~rid ~data =
>>>>>    (**
>>>>>     * Nothrow guarantee.
>>>>>     *)
>>>>> -let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
>>>>> +let process_packet ~store ~cons ~doms ~con ~req =
>>>>> +    let ty = req.Packet.ty in
>>>>> +    let tid = req.Packet.tid in
>>>>> +    let rid = req.Packet.rid in
>>>>>        try
>>>>>            let fct = function_of_type ty in
>>>>>            let t =
>>>>> @@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid
>>>>> ~rid ~ty ~data =
>>>>>                else
>>>>>                    Connection.get_transaction con tid
>>>>>                in
>>>>> -        let response = input_handle_error ~cons ~doms ~fct ~ty ~con
>>>>> ~t ~rid ~data in
>>>>> +        let response = input_handle_error ~cons ~doms ~fct ~con ~t
>>>>> ~req in
>>>>>              (* Put the response on the wire *)
>>>>>            send_response ty con t rid response
>>>>> @@ -412,11 +415,13 @@ let do_input store cons doms con =
>>>>>        if newpacket then (
>>>>>            let packet = Connection.pop_in con in
>>>>>            let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
>>>>> +        let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
>>>>> +
>>>> I think this change breaks the build with older ocaml versions:
>>>>
>>>> root@haswell> ocamlopt -v
>>>> The OCaml native-code compiler, version 4.00.1
>>>> Standard library directory: /usr/lib64/ocaml
>>>> root@haswell> ocamlopt -g -ccopt "    " -dtypes -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
>>>> -warn-error F -c -o process.cmx process.ml
>>>> root@haswell>
>>>>
>>>>
>>>> root@ovs104> ocamlopt -v
>>>> The Objective Caml native-code compiler, version 3.11.2
>>>> Standard library directory: /usr/lib64/ocaml
>>>> root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
>>>> -warn-error F -c -o process.cmx process.ml
>>>> File "process.ml", line 487, characters 23-24:
>>>> Error: Syntax error
>>>> root@ovs104>
>>>>
>>>> I don't know much about ocaml (OK, I know *nothing* about ocaml) so I
>>>> can't say what exactly might be wrong.
>>> Could you perhaps try this instead?
>>>
>>> let req = {tid = Packet.tid; rid = Packet.rid; ty = Packet.ty; data =
>>> Packet.data} in
>>>
>>> It is most likely that the older version of Ocaml can't infer the order
>>> of fields.
>>>
>>> ~Andrew
>>
>> No, it now gives me "Error: Unbound record field label tid"
>>
>> -boris
> Andrew's guess was close, but the wrong way around -- please could you try the
> following with the older compiler?
>
> let req = {Packet.tid=tid; Packet.rid=rid; Packet.ty=ty; Packet.data=data} in
>
> I was using a syntactic feature of OCaml called 'field punning' which is
> generally considered good practice and makes for more readable code. It looks
> like this feature was introduced in OCaml 3.12.0 (dating from 2010), which is
> consistent with Boris' findings.

Yes, this fixes it, thanks.

> What's the policy here -- is there a defined version of the OCaml compiler which
> tools/ocaml needs to be able to compile with?

Not to answer this question directly but more as an FYI --- this version 
of ocaml is found on Oracle Linux 6.2, which AFAIK is actively supported.

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/7] oxenstored: refactor request processing

[Wei Liu]

On Tue, Mar 29, 2016 at 10:08:30AM +0100, Jonathan Davies wrote:
> On Thu, Mar 24, 2016 at 07:57:30PM -0400, Boris Ostrovsky wrote:
> > On 03/24/2016 06:49 PM, Andrew Cooper wrote:
> > >On 24/03/2016 22:22, Boris Ostrovsky wrote:
> > >>On 03/17/2016 01:51 PM, Jonathan Davies wrote:
> > >>>Encapsulate the request in a record that is passed from do_input to
> > >>>process_packet and input_handle_error.
> > >>>
> > >>>This will be helpful when keeping track of the requests made as part
> > >>>of a
> > >>>transaction.
> > >>>
> > >>>Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
> > >>>Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > >>>Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
> > >>>Reviewed-by: Euan Harris <euan.harris@citrix.com>
> > >>>---
> > >>>   tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
> > >>>   1 file changed, 10 insertions(+), 5 deletions(-)
> > >>>
> > >>>diff --git a/tools/ocaml/xenstored/process.ml
> > >>>b/tools/ocaml/xenstored/process.ml
> > >>>index 7a73669..c92bec7 100644
> > >>>--- a/tools/ocaml/xenstored/process.ml
> > >>>+++ b/tools/ocaml/xenstored/process.ml
> > >>>@@ -344,11 +344,11 @@ let function_of_type ty =
> > >>>       | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
> > >>>       | _                              -> reply_ack do_error
> > >>>   -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
> > >>>+let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
> > >>>       let reply_error e =
> > >>>           Packet.Error e in
> > >>>       try
> > >>>-        fct con t doms cons data
> > >>>+        fct con t doms cons req.Packet.data
> > >>>       with
> > >>>       | Define.Invalid_path          -> reply_error "EINVAL"
> > >>>       | Define.Already_exist         -> reply_error "EEXIST"
> > >>>@@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con
> > >>>~t ~rid ~data =
> > >>>   (**
> > >>>    * Nothrow guarantee.
> > >>>    *)
> > >>>-let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
> > >>>+let process_packet ~store ~cons ~doms ~con ~req =
> > >>>+    let ty = req.Packet.ty in
> > >>>+    let tid = req.Packet.tid in
> > >>>+    let rid = req.Packet.rid in
> > >>>       try
> > >>>           let fct = function_of_type ty in
> > >>>           let t =
> > >>>@@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid
> > >>>~rid ~ty ~data =
> > >>>               else
> > >>>                   Connection.get_transaction con tid
> > >>>               in
> > >>>-        let response = input_handle_error ~cons ~doms ~fct ~ty ~con
> > >>>~t ~rid ~data in
> > >>>+        let response = input_handle_error ~cons ~doms ~fct ~con ~t
> > >>>~req in
> > >>>             (* Put the response on the wire *)
> > >>>           send_response ty con t rid response
> > >>>@@ -412,11 +415,13 @@ let do_input store cons doms con =
> > >>>       if newpacket then (
> > >>>           let packet = Connection.pop_in con in
> > >>>           let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
> > >>>+        let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
> > >>>+
> > >>I think this change breaks the build with older ocaml versions:
> > >>
> > >>root@haswell> ocamlopt -v
> > >>The OCaml native-code compiler, version 4.00.1
> > >>Standard library directory: /usr/lib64/ocaml
> > >>root@haswell> ocamlopt -g -ccopt "    " -dtypes -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> > >>-warn-error F -c -o process.cmx process.ml
> > >>root@haswell>
> > >>
> > >>
> > >>root@ovs104> ocamlopt -v
> > >>The Objective Caml native-code compiler, version 3.11.2
> > >>Standard library directory: /usr/lib64/ocaml
> > >>root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> > >>/root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> > >>-warn-error F -c -o process.cmx process.ml
> > >>File "process.ml", line 487, characters 23-24:
> > >>Error: Syntax error
> > >>root@ovs104>
> > >>
> > >>I don't know much about ocaml (OK, I know *nothing* about ocaml) so I
> > >>can't say what exactly might be wrong.
> > >Could you perhaps try this instead?
> > >
> > >let req = {tid = Packet.tid; rid = Packet.rid; ty = Packet.ty; data =
> > >Packet.data} in
> > >
> > >It is most likely that the older version of Ocaml can't infer the order
> > >of fields.
> > >
> > >~Andrew
> > 
> > 
> > No, it now gives me "Error: Unbound record field label tid"
> > 
> > -boris
> 
> Andrew's guess was close, but the wrong way around -- please could you try the
> following with the older compiler?
> 
> let req = {Packet.tid=tid; Packet.rid=rid; Packet.ty=ty; Packet.data=data} in
> 
> I was using a syntactic feature of OCaml called 'field punning' which is
> generally considered good practice and makes for more readable code. It looks
> like this feature was introduced in OCaml 3.12.0 (dating from 2010), which is
> consistent with Boris' findings.
> 
> What's the policy here -- is there a defined version of the OCaml compiler which
> tools/ocaml needs to be able to compile with?

It is not explicitly listed in README or INSTALL. The ocaml tools
maintainer (Dave in this case) is welcome to provide the minimum version
required.

Meanwhile, I don't think we should break existing build without pinning
down the minimum required version first, so we should fix Boris's
breakage.  The fix seems simple enough anyway.

Wei.

> 
> Thanks,
> Jonathan
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/7] oxenstored: refactor request processing

[David Scott]

> On 29 Mar 2016, at 17:38, Wei Liu <wei.liu2@citrix.com> wrote:
> 
> On Tue, Mar 29, 2016 at 10:08:30AM +0100, Jonathan Davies wrote:
>> On Thu, Mar 24, 2016 at 07:57:30PM -0400, Boris Ostrovsky wrote:
>>> On 03/24/2016 06:49 PM, Andrew Cooper wrote:
>>>> On 24/03/2016 22:22, Boris Ostrovsky wrote:
>>>>> On 03/17/2016 01:51 PM, Jonathan Davies wrote:
>>>>>> Encapsulate the request in a record that is passed from do_input to
>>>>>> process_packet and input_handle_error.
>>>>>> 
>>>>>> This will be helpful when keeping track of the requests made as part
>>>>>> of a
>>>>>> transaction.
>>>>>> 
>>>>>> Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
>>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>> Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
>>>>>> Reviewed-by: Euan Harris <euan.harris@citrix.com>
>>>>>> ---
>>>>>>  tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
>>>>>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>>>>> 
>>>>>> diff --git a/tools/ocaml/xenstored/process.ml
>>>>>> b/tools/ocaml/xenstored/process.ml
>>>>>> index 7a73669..c92bec7 100644
>>>>>> --- a/tools/ocaml/xenstored/process.ml
>>>>>> +++ b/tools/ocaml/xenstored/process.ml
>>>>>> @@ -344,11 +344,11 @@ let function_of_type ty =
>>>>>>      | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
>>>>>>      | _                              -> reply_ack do_error
>>>>>>  -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
>>>>>> +let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
>>>>>>      let reply_error e =
>>>>>>          Packet.Error e in
>>>>>>      try
>>>>>> -        fct con t doms cons data
>>>>>> +        fct con t doms cons req.Packet.data
>>>>>>      with
>>>>>>      | Define.Invalid_path          -> reply_error "EINVAL"
>>>>>>      | Define.Already_exist         -> reply_error "EEXIST"
>>>>>> @@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con
>>>>>> ~t ~rid ~data =
>>>>>>  (**
>>>>>>   * Nothrow guarantee.
>>>>>>   *)
>>>>>> -let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
>>>>>> +let process_packet ~store ~cons ~doms ~con ~req =
>>>>>> +    let ty = req.Packet.ty in
>>>>>> +    let tid = req.Packet.tid in
>>>>>> +    let rid = req.Packet.rid in
>>>>>>      try
>>>>>>          let fct = function_of_type ty in
>>>>>>          let t =
>>>>>> @@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid
>>>>>> ~rid ~ty ~data =
>>>>>>              else
>>>>>>                  Connection.get_transaction con tid
>>>>>>              in
>>>>>> -        let response = input_handle_error ~cons ~doms ~fct ~ty ~con
>>>>>> ~t ~rid ~data in
>>>>>> +        let response = input_handle_error ~cons ~doms ~fct ~con ~t
>>>>>> ~req in
>>>>>>            (* Put the response on the wire *)
>>>>>>          send_response ty con t rid response
>>>>>> @@ -412,11 +415,13 @@ let do_input store cons doms con =
>>>>>>      if newpacket then (
>>>>>>          let packet = Connection.pop_in con in
>>>>>>          let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
>>>>>> +        let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
>>>>>> +
>>>>> I think this change breaks the build with older ocaml versions:
>>>>> 
>>>>> root@haswell> ocamlopt -v
>>>>> The OCaml native-code compiler, version 4.00.1
>>>>> Standard library directory: /usr/lib64/ocaml
>>>>> root@haswell> ocamlopt -g -ccopt "    " -dtypes -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
>>>>> -warn-error F -c -o process.cmx process.ml
>>>>> root@haswell>
>>>>> 
>>>>> 
>>>>> root@ovs104> ocamlopt -v
>>>>> The Objective Caml native-code compiler, version 3.11.2
>>>>> Standard library directory: /usr/lib64/ocaml
>>>>> root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
>>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
>>>>> -warn-error F -c -o process.cmx process.ml
>>>>> File "process.ml", line 487, characters 23-24:
>>>>> Error: Syntax error
>>>>> root@ovs104>
>>>>> 
>>>>> I don't know much about ocaml (OK, I know *nothing* about ocaml) so I
>>>>> can't say what exactly might be wrong.
>>>> Could you perhaps try this instead?
>>>> 
>>>> let req = {tid = Packet.tid; rid = Packet.rid; ty = Packet.ty; data =
>>>> Packet.data} in
>>>> 
>>>> It is most likely that the older version of Ocaml can't infer the order
>>>> of fields.
>>>> 
>>>> ~Andrew
>>> 
>>> 
>>> No, it now gives me "Error: Unbound record field label tid"
>>> 
>>> -boris
>> 
>> Andrew's guess was close, but the wrong way around -- please could you try the
>> following with the older compiler?
>> 
>> let req = {Packet.tid=tid; Packet.rid=rid; Packet.ty=ty; Packet.data=data} in
>> 
>> I was using a syntactic feature of OCaml called 'field punning' which is
>> generally considered good practice and makes for more readable code. It looks
>> like this feature was introduced in OCaml 3.12.0 (dating from 2010), which is
>> consistent with Boris' findings.
>> 
>> What's the policy here -- is there a defined version of the OCaml compiler which
>> tools/ocaml needs to be able to compile with?
> 
> It is not explicitly listed in README or INSTALL. The ocaml tools
> maintainer (Dave in this case) is welcome to provide the minimum version
> required.
> 
> Meanwhile, I don't think we should break existing build without pinning
> down the minimum required version first, so we should fix Boris's
> breakage.  The fix seems simple enough anyway.

It looks like the fix is small and easy — I think this is good for now.

Let’s postpone requiring a later OCaml version until we really need a feature only present in a later version. I suspect this will happen eventually, probably when we try to add a dependency (e.g. from the Mirage world) which requires 4.02+.

Cheers,
Dave

> 
> Wei.
> 
>> 
>> Thanks,
>> Jonathan
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/7] oxenstored: refactor request processing

[Jonathan Davies]

On Tue, Mar 29, 2016 at 08:41:54PM +0100, David Scott wrote:
> 
> > On 29 Mar 2016, at 17:38, Wei Liu <wei.liu2@citrix.com> wrote:
> > 
> > On Tue, Mar 29, 2016 at 10:08:30AM +0100, Jonathan Davies wrote:
> >> On Thu, Mar 24, 2016 at 07:57:30PM -0400, Boris Ostrovsky wrote:
> >>> On 03/24/2016 06:49 PM, Andrew Cooper wrote:
> >>>> On 24/03/2016 22:22, Boris Ostrovsky wrote:
> >>>>> On 03/17/2016 01:51 PM, Jonathan Davies wrote:
> >>>>>> Encapsulate the request in a record that is passed from do_input to
> >>>>>> process_packet and input_handle_error.
> >>>>>> 
> >>>>>> This will be helpful when keeping track of the requests made as part
> >>>>>> of a
> >>>>>> transaction.
> >>>>>> 
> >>>>>> Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
> >>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>>>> Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
> >>>>>> Reviewed-by: Euan Harris <euan.harris@citrix.com>
> >>>>>> ---
> >>>>>>  tools/ocaml/xenstored/process.ml |   15 ++++++++++-----
> >>>>>>  1 file changed, 10 insertions(+), 5 deletions(-)
> >>>>>> 
> >>>>>> diff --git a/tools/ocaml/xenstored/process.ml
> >>>>>> b/tools/ocaml/xenstored/process.ml
> >>>>>> index 7a73669..c92bec7 100644
> >>>>>> --- a/tools/ocaml/xenstored/process.ml
> >>>>>> +++ b/tools/ocaml/xenstored/process.ml
> >>>>>> @@ -344,11 +344,11 @@ let function_of_type ty =
> >>>>>>      | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
> >>>>>>      | _                              -> reply_ack do_error
> >>>>>>  -let input_handle_error ~cons ~doms ~fct ~ty ~con ~t ~rid ~data =
> >>>>>> +let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
> >>>>>>      let reply_error e =
> >>>>>>          Packet.Error e in
> >>>>>>      try
> >>>>>> -        fct con t doms cons data
> >>>>>> +        fct con t doms cons req.Packet.data
> >>>>>>      with
> >>>>>>      | Define.Invalid_path          -> reply_error "EINVAL"
> >>>>>>      | Define.Already_exist         -> reply_error "EEXIST"
> >>>>>> @@ -370,7 +370,10 @@ let input_handle_error ~cons ~doms ~fct ~ty ~con
> >>>>>> ~t ~rid ~data =
> >>>>>>  (**
> >>>>>>   * Nothrow guarantee.
> >>>>>>   *)
> >>>>>> -let process_packet ~store ~cons ~doms ~con ~tid ~rid ~ty ~data =
> >>>>>> +let process_packet ~store ~cons ~doms ~con ~req =
> >>>>>> +    let ty = req.Packet.ty in
> >>>>>> +    let tid = req.Packet.tid in
> >>>>>> +    let rid = req.Packet.rid in
> >>>>>>      try
> >>>>>>          let fct = function_of_type ty in
> >>>>>>          let t =
> >>>>>> @@ -379,7 +382,7 @@ let process_packet ~store ~cons ~doms ~con ~tid
> >>>>>> ~rid ~ty ~data =
> >>>>>>              else
> >>>>>>                  Connection.get_transaction con tid
> >>>>>>              in
> >>>>>> -        let response = input_handle_error ~cons ~doms ~fct ~ty ~con
> >>>>>> ~t ~rid ~data in
> >>>>>> +        let response = input_handle_error ~cons ~doms ~fct ~con ~t
> >>>>>> ~req in
> >>>>>>            (* Put the response on the wire *)
> >>>>>>          send_response ty con t rid response
> >>>>>> @@ -412,11 +415,13 @@ let do_input store cons doms con =
> >>>>>>      if newpacket then (
> >>>>>>          let packet = Connection.pop_in con in
> >>>>>>          let tid, rid, ty, data = Xenbus.Xb.Packet.unpack packet in
> >>>>>> +        let req = {Packet.tid; Packet.rid; Packet.ty; Packet.data} in
> >>>>>> +
> >>>>> I think this change breaks the build with older ocaml versions:
> >>>>> 
> >>>>> root@haswell> ocamlopt -v
> >>>>> The OCaml native-code compiler, version 4.00.1
> >>>>> Standard library directory: /usr/lib64/ocaml
> >>>>> root@haswell> ocamlopt -g -ccopt "    " -dtypes -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> >>>>> -warn-error F -c -o process.cmx process.ml
> >>>>> root@haswell>
> >>>>> 
> >>>>> 
> >>>>> root@ovs104> ocamlopt -v
> >>>>> The Objective Caml native-code compiler, version 3.11.2
> >>>>> Standard library directory: /usr/lib64/ocaml
> >>>>> root@ovs104> ocamlopt -g -ccopt "    " -dtypes -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xb -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/mmap -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/xc -I
> >>>>> /root/tmp/xen/tools/ocaml/xenstored/../libs/eventchn -cc gcc -w F
> >>>>> -warn-error F -c -o process.cmx process.ml
> >>>>> File "process.ml", line 487, characters 23-24:
> >>>>> Error: Syntax error
> >>>>> root@ovs104>
> >>>>> 
> >>>>> I don't know much about ocaml (OK, I know *nothing* about ocaml) so I
> >>>>> can't say what exactly might be wrong.
> >>>> Could you perhaps try this instead?
> >>>> 
> >>>> let req = {tid = Packet.tid; rid = Packet.rid; ty = Packet.ty; data =
> >>>> Packet.data} in
> >>>> 
> >>>> It is most likely that the older version of Ocaml can't infer the order
> >>>> of fields.
> >>>> 
> >>>> ~Andrew
> >>> 
> >>> 
> >>> No, it now gives me "Error: Unbound record field label tid"
> >>> 
> >>> -boris
> >> 
> >> Andrew's guess was close, but the wrong way around -- please could you try the
> >> following with the older compiler?
> >> 
> >> let req = {Packet.tid=tid; Packet.rid=rid; Packet.ty=ty; Packet.data=data} in
> >> 
> >> I was using a syntactic feature of OCaml called 'field punning' which is
> >> generally considered good practice and makes for more readable code. It looks
> >> like this feature was introduced in OCaml 3.12.0 (dating from 2010), which is
> >> consistent with Boris' findings.
> >> 
> >> What's the policy here -- is there a defined version of the OCaml compiler which
> >> tools/ocaml needs to be able to compile with?
> > 
> > It is not explicitly listed in README or INSTALL. The ocaml tools
> > maintainer (Dave in this case) is welcome to provide the minimum version
> > required.
> > 
> > Meanwhile, I don't think we should break existing build without pinning
> > down the minimum required version first, so we should fix Boris's
> > breakage.  The fix seems simple enough anyway.
> 
> It looks like the fix is small and easy — I think this is good for now.
> 
> Let’s postpone requiring a later OCaml version until we really need a feature only present in a later version. I suspect this will happen eventually, probably when we try to add a dependency (e.g. from the Mirage world) which requires 4.02+.

OK; sounds sensible.

Since the original patch has already been committed to staging, I presume you'd
like me to formally submit a standalone patch that fixes this issue. I will post
it separately.

If I'm wrong and you'd like me to post a v2 of the whole series, let me know.

Thanks,
Jonathan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/7] oxenstored: refactor request processing

[Wei Liu]

On Wed, Mar 30, 2016 at 04:46:58PM +0100, Jonathan Davies wrote:
[...]
> > >> Andrew's guess was close, but the wrong way around -- please could you try the
> > >> following with the older compiler?
> > >> 
> > >> let req = {Packet.tid=tid; Packet.rid=rid; Packet.ty=ty; Packet.data=data} in
> > >> 
> > >> I was using a syntactic feature of OCaml called 'field punning' which is
> > >> generally considered good practice and makes for more readable code. It looks
> > >> like this feature was introduced in OCaml 3.12.0 (dating from 2010), which is
> > >> consistent with Boris' findings.
> > >> 
> > >> What's the policy here -- is there a defined version of the OCaml compiler which
> > >> tools/ocaml needs to be able to compile with?
> > > 
> > > It is not explicitly listed in README or INSTALL. The ocaml tools
> > > maintainer (Dave in this case) is welcome to provide the minimum version
> > > required.
> > > 
> > > Meanwhile, I don't think we should break existing build without pinning
> > > down the minimum required version first, so we should fix Boris's
> > > breakage.  The fix seems simple enough anyway.
> > 
> > It looks like the fix is small and easy — I think this is good for now.
> > 
> > Let’s postpone requiring a later OCaml version until we really need a feature only present in a later version. I suspect this will happen eventually, probably when we try to add a dependency (e.g. from the Mirage world) which requires 4.02+.
> 
> OK; sounds sensible.
> 
> Since the original patch has already been committed to staging, I presume you'd
> like me to formally submit a standalone patch that fixes this issue. I will post
> it separately.
> 

Yes, a standalone patch to fix the issue is fine.

Wei.

> If I'm wrong and you'd like me to post a v2 of the whole series, let me know.
> 
> Thanks,
> Jonathan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 4/7] oxenstored: keep track of each transaction's operations

[Jonathan Davies]

A list of (request, response) pairs from the operations performed within the
transaction will be useful to support transaction replay.

Since this consumes memory, the number of requests per transaction must not be
left unbounded. Hence a new quota for this is introduced. This quota, configured
via the configuration key 'quota-maxrequests', limits the size of transactions
initiated by domUs.

After the maximum number of requests has been exhausted, any further requests
will result in EQUOTA errors. The client may then choose to end the transaction;
a successful commit will result in the retention of only the prior requests.

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Reviewed-by: Euan Harris <euan.harris@citrix.com>
---
 tools/ocaml/xenstored/define.ml       |    1 +
 tools/ocaml/xenstored/oxenstored.conf |    1 +
 tools/ocaml/xenstored/process.ml      |   13 +++++++++++--
 tools/ocaml/xenstored/transaction.ml  |   21 +++++++++++++++------
 tools/ocaml/xenstored/xenstored.ml    |    1 +
 5 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/tools/ocaml/xenstored/define.ml b/tools/ocaml/xenstored/define.ml
index 89a6aac..d60861c 100644
--- a/tools/ocaml/xenstored/define.ml
+++ b/tools/ocaml/xenstored/define.ml
@@ -27,6 +27,7 @@ let default_config_dir = "/etc/xen"
 
 let maxwatch = ref (50)
 let maxtransaction = ref (20)
+let maxrequests = ref (-1)   (* maximum requests per transaction *)
 
 let domid_self = 0x7FF0
 
diff --git a/tools/ocaml/xenstored/oxenstored.conf b/tools/ocaml/xenstored/oxenstored.conf
index dd20eda..ac60f49 100644
--- a/tools/ocaml/xenstored/oxenstored.conf
+++ b/tools/ocaml/xenstored/oxenstored.conf
@@ -18,6 +18,7 @@ quota-maxentity = 1000
 quota-maxsize = 2048
 quota-maxwatch = 100
 quota-transaction = 10
+quota-maxrequests = 1024
 
 # Activate filed base backend
 persistent = false
diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index c92bec7..758ade1 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -155,7 +155,7 @@ let do_transaction_end con t domains cons data =
 	if not success then
 		raise Transaction_again;
 	if commit then
-		process_watch (List.rev (Transaction.get_ops t)) cons
+		process_watch (List.rev (Transaction.get_paths t)) cons
 
 let do_introduce con t domains cons data =
 	if not (Connection.is_dom0 con)
@@ -303,7 +303,7 @@ let reply_ack fct con t doms cons data =
 	fct con t doms cons data;
 	Packet.Ack (fun () ->
 		if Transaction.get_id t = Transaction.none then
-			process_watch (Transaction.get_ops t) cons
+			process_watch (Transaction.get_paths t) cons
 	)
 
 let reply_data fct con t doms cons data =
@@ -384,6 +384,15 @@ let process_packet ~store ~cons ~doms ~con ~req =
 			in
 		let response = input_handle_error ~cons ~doms ~fct ~con ~t ~req in
 
+		let response = try
+			if tid <> Transaction.none then
+				(* Remember the request and response for this operation in case we need to replay the transaction *)
+				Transaction.add_operation ~perm:(Connection.get_perm con) t req response;
+			response
+		with Quota.Limit_reached ->
+			Packet.Error "EQUOTA"
+		in
+
 		(* Put the response on the wire *)
 		send_response ty con t rid response
 	with exn ->
diff --git a/tools/ocaml/xenstored/transaction.ml b/tools/ocaml/xenstored/transaction.ml
index 77de4e8..6b37fc2 100644
--- a/tools/ocaml/xenstored/transaction.ml
+++ b/tools/ocaml/xenstored/transaction.ml
@@ -75,7 +75,8 @@ type t = {
 	ty: ty;
 	store: Store.t;
 	quota: Quota.t;
-	mutable ops: (Xenbus.Xb.Op.operation * Store.Path.t) list;
+	mutable paths: (Xenbus.Xb.Op.operation * Store.Path.t) list;
+	mutable operations: (Packet.request * Packet.response) list;
 	mutable read_lowpath: Store.Path.t option;
 	mutable write_lowpath: Store.Path.t option;
 }
@@ -86,16 +87,24 @@ let make id store =
 		ty = ty;
 		store = if id = none then store else Store.copy store;
 		quota = Quota.copy store.Store.quota;
-		ops = [];
+		paths = [];
+		operations = [];
 		read_lowpath = None;
 		write_lowpath = None;
 	}
 
 let get_id t = match t.ty with No -> none | Full (id, _, _) -> id
 let get_store t = t.store
-let get_ops t = t.ops
-
-let add_wop t ty path = t.ops <- (ty, path) :: t.ops
+let get_paths t = t.paths
+
+let add_wop t ty path = t.paths <- (ty, path) :: t.paths
+let add_operation ~perm t request response =
+	if !Define.maxrequests >= 0
+		&& not (Perms.Connection.is_dom0 perm)
+		&& List.length t.operations >= !Define.maxrequests
+		then raise Quota.Limit_reached;
+	t.operations <- (request, response) :: t.operations
+let get_operations t = List.rev t.operations
 let set_read_lowpath t path = t.read_lowpath <- get_lowest path t.read_lowpath
 let set_write_lowpath t path = t.write_lowpath <- get_lowest path t.write_lowpath
 
@@ -141,7 +150,7 @@ let getperms t perm path =
 	r
 
 let commit ~con t =
-	let has_write_ops = List.length t.ops > 0 in
+	let has_write_ops = List.length t.paths > 0 in
 	let has_coalesced = ref false in
 	let has_commited =
 	match t.ty with
diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index 25c126a..fc8cc95 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -95,6 +95,7 @@ let parse_config filename =
 		("quota-transaction", Config.Set_int Define.maxtransaction);
 		("quota-maxentity", Config.Set_int Quota.maxent);
 		("quota-maxsize", Config.Set_int Quota.maxsize);
+		("quota-maxrequests", Config.Set_int Define.maxrequests);
 		("test-eagain", Config.Set_bool Transaction.test_eagain);
 		("persistent", Config.Set_bool Disk.enable);
 		("xenstored-log-file", Config.String Logging.set_xenstored_log_destination);
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 5/7] oxenstored: move functions that process simple operations

[Jonathan Davies]

Separate the functions which process operations that can be done as part of a
transaction. Specifically, these operations are: read, write, rm, getperms,
setperms, getdomainpath, directory, mkdir.

Also split function_of_type into two functions: one for processing the simple
operations and one for processing the rest.

This will help allow replay of transactions, allowing us to invoke the functions
that process the simple operations as part of the processing of transaction_end.

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Reviewed-by: Euan Harris <euan.harris@citrix.com>
---
 tools/ocaml/xenstored/process.ml |  223 +++++++++++++++++++++-----------------
 1 file changed, 121 insertions(+), 102 deletions(-)

diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index 758ade1..39ae71b 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -119,94 +119,6 @@ let do_getperms con t domains cons data =
 	let perms = Transaction.getperms t (Connection.get_perm con) path in
 	Perms.Node.to_string perms ^ "\000"
 
-let do_watch con t domains cons data =
-	let (node, token) = 
-		match (split None '\000' data) with
-		| [node; token; ""]   -> node, token
-		| _                   -> raise Invalid_Cmd_Args
-		in
-	let watch = Connections.add_watch cons con node token in
-	Packet.Ack (fun () -> Connection.fire_single_watch watch)
-
-let do_unwatch con t domains cons data =
-	let (node, token) =
-		match (split None '\000' data) with
-		| [node; token; ""]   -> node, token
-		| _                   -> raise Invalid_Cmd_Args
-		in
-	Connections.del_watch cons con node token
-
-let do_transaction_start con t domains cons data =
-	if Transaction.get_id t <> Transaction.none then
-		raise Transaction_nested;
-	let store = Transaction.get_store t in
-	string_of_int (Connection.start_transaction con store) ^ "\000"
-
-let do_transaction_end con t domains cons data =
-	let commit =
-		match (split None '\000' data) with
-		| "T" :: _ -> true
-		| "F" :: _ -> false
-		| x :: _   -> raise (Invalid_argument x)
-		| _        -> raise Invalid_Cmd_Args
-		in
-	let success =
-		Connection.end_transaction con (Transaction.get_id t) commit in
-	if not success then
-		raise Transaction_again;
-	if commit then
-		process_watch (List.rev (Transaction.get_paths t)) cons
-
-let do_introduce con t domains cons data =
-	if not (Connection.is_dom0 con)
-	then raise Define.Permission_denied;
-	let (domid, mfn, port) =
-		match (split None '\000' data) with
-		| domid :: mfn :: port :: _ ->
-			int_of_string domid, Nativeint.of_string mfn, int_of_string port
-		| _                         -> raise Invalid_Cmd_Args;
-		in
-	let dom =
-		if Domains.exist domains domid then
-			Domains.find domains domid
-		else try
-			let ndom = Xenctrl.with_intf (fun xc ->
-				Domains.create xc domains domid mfn port) in
-			Connections.add_domain cons ndom;
-			Connections.fire_spec_watches cons "@introduceDomain";
-			ndom
-		with _ -> raise Invalid_Cmd_Args
-	in
-	if (Domain.get_remote_port dom) <> port || (Domain.get_mfn dom) <> mfn then
-		raise Domain_not_match
-
-let do_release con t domains cons data =
-	if not (Connection.is_dom0 con)
-	then raise Define.Permission_denied;
-	let domid =
-		match (split None '\000' data) with
-		| [domid;""] -> int_of_string domid
-		| _          -> raise Invalid_Cmd_Args
-		in
-	let fire_spec_watches = Domains.exist domains domid in
-	Domains.del domains domid;
-	Connections.del_domain cons domid;
-	if fire_spec_watches 
-	then Connections.fire_spec_watches cons "@releaseDomain"
-	else raise Invalid_Cmd_Args
-
-let do_resume con t domains cons data =
-	if not (Connection.is_dom0 con)
-	then raise Define.Permission_denied;
-	let domid =
-		match (split None '\000' data) with
-		| domid :: _ -> int_of_string domid
-		| _          -> raise Invalid_Cmd_Args
-		in
-	if Domains.exist domains domid
-	then Domains.resume domains domid
-	else raise Invalid_Cmd_Args
-
 let do_getdomainpath con t domains cons data =
 	let domid =
 		match (split None '\000' data) with
@@ -319,29 +231,31 @@ let reply_none fct con t doms cons data =
 	(* let the function reply *)
 	fct con t doms cons data
 
-let function_of_type ty =
+(* Functions for 'simple' operations that cannot be part of a transaction *)
+let function_of_type_simple_op ty =
 	match ty with
-	| Xenbus.Xb.Op.Debug             -> reply_data_or_ack do_debug
+	| Xenbus.Xb.Op.Debug
+	| Xenbus.Xb.Op.Watch
+	| Xenbus.Xb.Op.Unwatch
+	| Xenbus.Xb.Op.Transaction_start
+	| Xenbus.Xb.Op.Transaction_end
+	| Xenbus.Xb.Op.Introduce
+	| Xenbus.Xb.Op.Release
+	| Xenbus.Xb.Op.Isintroduced
+	| Xenbus.Xb.Op.Resume
+	| Xenbus.Xb.Op.Set_target
+	| Xenbus.Xb.Op.Restrict
+	| Xenbus.Xb.Op.Reset_watches
+	| Xenbus.Xb.Op.Invalid           -> error "called function_of_type_simple_op on operation %s" (Xenbus.Xb.Op.to_string ty);
+	                                    raise (Invalid_argument (Xenbus.Xb.Op.to_string ty))
 	| Xenbus.Xb.Op.Directory         -> reply_data do_directory
 	| Xenbus.Xb.Op.Read              -> reply_data do_read
 	| Xenbus.Xb.Op.Getperms          -> reply_data do_getperms
-	| Xenbus.Xb.Op.Watch             -> reply_none do_watch
-	| Xenbus.Xb.Op.Unwatch           -> reply_ack do_unwatch
-	| Xenbus.Xb.Op.Transaction_start -> reply_data do_transaction_start
-	| Xenbus.Xb.Op.Transaction_end   -> reply_ack do_transaction_end
-	| Xenbus.Xb.Op.Introduce         -> reply_ack do_introduce
-	| Xenbus.Xb.Op.Release           -> reply_ack do_release
 	| Xenbus.Xb.Op.Getdomainpath     -> reply_data do_getdomainpath
 	| Xenbus.Xb.Op.Write             -> reply_ack do_write
 	| Xenbus.Xb.Op.Mkdir             -> reply_ack do_mkdir
 	| Xenbus.Xb.Op.Rm                -> reply_ack do_rm
 	| Xenbus.Xb.Op.Setperms          -> reply_ack do_setperms
-	| Xenbus.Xb.Op.Isintroduced      -> reply_data do_isintroduced
-	| Xenbus.Xb.Op.Resume            -> reply_ack do_resume
-	| Xenbus.Xb.Op.Set_target        -> reply_ack do_set_target
-	| Xenbus.Xb.Op.Restrict          -> reply_ack do_restrict
-	| Xenbus.Xb.Op.Reset_watches     -> reply_ack do_reset_watches
-	| Xenbus.Xb.Op.Invalid           -> reply_ack do_error
 	| _                              -> reply_ack do_error
 
 let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
@@ -367,6 +281,111 @@ let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
 	| (Failure "int_of_string")    -> reply_error "EINVAL"
 	| Define.Unknown_operation     -> reply_error "ENOSYS"
 
+let do_watch con t domains cons data =
+	let (node, token) = 
+		match (split None '\000' data) with
+		| [node; token; ""]   -> node, token
+		| _                   -> raise Invalid_Cmd_Args
+		in
+	let watch = Connections.add_watch cons con node token in
+	Packet.Ack (fun () -> Connection.fire_single_watch watch)
+
+let do_unwatch con t domains cons data =
+	let (node, token) =
+		match (split None '\000' data) with
+		| [node; token; ""]   -> node, token
+		| _                   -> raise Invalid_Cmd_Args
+		in
+	Connections.del_watch cons con node token
+
+let do_transaction_start con t domains cons data =
+	if Transaction.get_id t <> Transaction.none then
+		raise Transaction_nested;
+	let store = Transaction.get_store t in
+	string_of_int (Connection.start_transaction con store) ^ "\000"
+
+let do_transaction_end con t domains cons data =
+	let commit =
+		match (split None '\000' data) with
+		| "T" :: _ -> true
+		| "F" :: _ -> false
+		| x :: _   -> raise (Invalid_argument x)
+		| _        -> raise Invalid_Cmd_Args
+		in
+	let success =
+		Connection.end_transaction con (Transaction.get_id t) commit in
+	if not success then
+		raise Transaction_again;
+	if commit then
+		process_watch (List.rev (Transaction.get_paths t)) cons
+
+let do_introduce con t domains cons data =
+	if not (Connection.is_dom0 con)
+	then raise Define.Permission_denied;
+	let (domid, mfn, port) =
+		match (split None '\000' data) with
+		| domid :: mfn :: port :: _ ->
+			int_of_string domid, Nativeint.of_string mfn, int_of_string port
+		| _                         -> raise Invalid_Cmd_Args;
+		in
+	let dom =
+		if Domains.exist domains domid then
+			Domains.find domains domid
+		else try
+			let ndom = Xenctrl.with_intf (fun xc ->
+				Domains.create xc domains domid mfn port) in
+			Connections.add_domain cons ndom;
+			Connections.fire_spec_watches cons "@introduceDomain";
+			ndom
+		with _ -> raise Invalid_Cmd_Args
+	in
+	if (Domain.get_remote_port dom) <> port || (Domain.get_mfn dom) <> mfn then
+		raise Domain_not_match
+
+let do_release con t domains cons data =
+	if not (Connection.is_dom0 con)
+	then raise Define.Permission_denied;
+	let domid =
+		match (split None '\000' data) with
+		| [domid;""] -> int_of_string domid
+		| _          -> raise Invalid_Cmd_Args
+		in
+	let fire_spec_watches = Domains.exist domains domid in
+	Domains.del domains domid;
+	Connections.del_domain cons domid;
+	if fire_spec_watches 
+	then Connections.fire_spec_watches cons "@releaseDomain"
+	else raise Invalid_Cmd_Args
+
+let do_resume con t domains cons data =
+	if not (Connection.is_dom0 con)
+	then raise Define.Permission_denied;
+	let domid =
+		match (split None '\000' data) with
+		| domid :: _ -> int_of_string domid
+		| _          -> raise Invalid_Cmd_Args
+		in
+	if Domains.exist domains domid
+	then Domains.resume domains domid
+	else raise Invalid_Cmd_Args
+
+let function_of_type ty =
+	match ty with
+	| Xenbus.Xb.Op.Debug             -> reply_data_or_ack do_debug
+	| Xenbus.Xb.Op.Watch             -> reply_none do_watch
+	| Xenbus.Xb.Op.Unwatch           -> reply_ack do_unwatch
+	| Xenbus.Xb.Op.Transaction_start -> reply_data do_transaction_start
+	| Xenbus.Xb.Op.Transaction_end   -> reply_ack do_transaction_end
+	| Xenbus.Xb.Op.Introduce         -> reply_ack do_introduce
+	| Xenbus.Xb.Op.Release           -> reply_ack do_release
+	| Xenbus.Xb.Op.Isintroduced      -> reply_data do_isintroduced
+	| Xenbus.Xb.Op.Resume            -> reply_ack do_resume
+	| Xenbus.Xb.Op.Set_target        -> reply_ack do_set_target
+	| Xenbus.Xb.Op.Restrict          -> reply_ack do_restrict
+	| Xenbus.Xb.Op.Reset_watches     -> reply_ack do_reset_watches
+	| Xenbus.Xb.Op.Invalid           -> reply_ack do_error
+	| _                              -> function_of_type_simple_op ty
+
 (**
  * Nothrow guarantee.
  *)
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 6/7] oxenstored: replay transaction upon conflict

[Jonathan Davies]

The existing transaction merge algorithm keeps track of the least upper bound
(longest common prefix) of all the nodes which have been read and written, and
will re-combine two stores which have disjoint upper bounds. This works well for
small transactions but causes unnecessary conflicts for ones that span a large
subtree, such as the following ones used by the xapi toolstack:

 * VM start: creates /vm/... /vss/... /local/domain/...
   The least upper bound of this transaction is / and so all
   these transactions conflict with everything.

 * Device hotplug: creates /local/domain/0/... /local/domain/n/...
   The least upper bound of this transaction is /local/domain so
   all these transactions conflict with each other.

If the existing merge algorithm cannot merge and commit, we attempt
a /replay/ of the failed transaction against the new store.

When we replay the requests we check whether the response sent to the client is
the same as during the first attempt at the transaction. If the responses are
all the same then the transaction replay can be committed. If any differ then
the transaction replay must be aborted and the client must retry.

This algorithm uses the intuition that the transactions made by the toolstack
are designed to be for separate domains, and should fundamentally not conflict
in the sense that they don't read or write any shared keys. By replaying the
transaction on the server side we do what the client would have to do anyway,
only we can do it quickly without allowing any other requests to interfere.

Performing 300 parallel simulated VM start and shutdowns without this code:

300 parallel starts and shutdowns: 268.92

Performing 300 parallel simulated VM start and shutdowns with this code:

300 parallel starts and shutdowns: 3.80

Signed-off-by: Dave Scott <dave@recoil.org>
Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Reviewed-by: Euan Harris <euan.harris@citrix.com>
---
 tools/ocaml/xenstored/connection.ml |    5 ++++-
 tools/ocaml/xenstored/process.ml    |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/tools/ocaml/xenstored/connection.ml b/tools/ocaml/xenstored/connection.ml
index 0a2c481..b18336f 100644
--- a/tools/ocaml/xenstored/connection.ml
+++ b/tools/ocaml/xenstored/connection.ml
@@ -233,7 +233,10 @@ let end_transaction con tid commit =
 	let trans = Hashtbl.find con.transactions tid in
 	Hashtbl.remove con.transactions tid;
 	Logging.end_transaction ~tid ~con:(get_domstr con);
-	if commit then Transaction.commit ~con:(get_domstr con) trans else true
+	match commit with
+	| None -> true
+	| Some transaction_replay_f ->
+		Transaction.commit ~con:(get_domstr con) trans || transaction_replay_f con trans
 
 let get_transaction con tid =
 	Hashtbl.find con.transactions tid
diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index 39ae71b..6d1f551 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -281,6 +281,38 @@ let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
 	| (Failure "int_of_string")    -> reply_error "EINVAL"
 	| Define.Unknown_operation     -> reply_error "ENOSYS"
 
+(* Replay a stored transaction against a fresh store, check the responses are
+   all equivalent: if so, commit the transaction. Otherwise send the abort to
+   the client. *)
+let transaction_replay c t doms cons =
+	match t.Transaction.ty with
+	| Transaction.No ->
+		error "attempted to replay a non-full transaction";
+		false
+	| Transaction.Full(id, oldroot, cstore) ->
+		let tid = Connection.start_transaction c cstore in
+		let new_t = Transaction.make tid cstore in
+		let con = sprintf "r(%d):%s" id (Connection.get_domstr c) in
+		let perform_exn (request, response) =
+			let fct = function_of_type_simple_op request.Packet.ty in
+			let response' = input_handle_error ~cons ~doms ~fct ~con:c ~t:new_t ~req:request in
+			if not(Packet.response_equal response response') then raise Transaction_again in
+		finally
+		(fun () ->
+			try
+				Logging.start_transaction ~con ~tid;
+				List.iter perform_exn (Transaction.get_operations t);
+				Logging.end_transaction ~con ~tid;
+
+				Transaction.commit ~con new_t
+			with e ->
+				info "transaction_replay %d caught: %s" tid (Printexc.to_string e);
+				false
+			)
+		(fun () ->
+			Connection.end_transaction c tid None
+		)
+
 let do_watch con t domains cons data =
 	let (node, token) = 
 		match (split None '\000' data) with
@@ -313,6 +345,7 @@ let do_transaction_end con t domains cons data =
 		| _        -> raise Invalid_Cmd_Args
 		in
 	let success =
+		let commit = if commit then Some (fun con trans -> transaction_replay con trans domains cons) else None in
 		Connection.end_transaction con (Transaction.get_id t) commit in
 	if not success then
 		raise Transaction_again;
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 7/7] oxenstored: log request and response during transaction replay

[Jonathan Davies]

During a transaction replay, the replayed requests and the new responses are
logged in the same way as the original requests and the original responses.

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Reviewed-by: Euan Harris <euan.harris@citrix.com>
---
 tools/ocaml/xenstored/process.ml |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index 6d1f551..fb5fdaf 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -281,6 +281,18 @@ let input_handle_error ~cons ~doms ~fct ~con ~t ~req =
 	| (Failure "int_of_string")    -> reply_error "EINVAL"
 	| Define.Unknown_operation     -> reply_error "ENOSYS"
 
+let write_access_log ~ty ~tid ~con ~data =
+	Logging.xb_op ~ty ~tid ~con data
+
+let write_answer_log ~ty ~tid ~con ~data =
+	Logging.xb_answer ~ty ~tid ~con data
+
+let write_response_log ~ty ~tid ~con ~response =
+	match response with
+	| Packet.Ack _   -> write_answer_log ~ty ~tid ~con ~data:""
+	| Packet.Reply x -> write_answer_log ~ty ~tid ~con ~data:x
+	| Packet.Error e -> write_answer_log ~ty:(Xenbus.Xb.Op.Error) ~tid ~con ~data:e
+
 (* Replay a stored transaction against a fresh store, check the responses are
    all equivalent: if so, commit the transaction. Otherwise send the abort to
    the client. *)
@@ -294,8 +306,10 @@ let transaction_replay c t doms cons =
 		let new_t = Transaction.make tid cstore in
 		let con = sprintf "r(%d):%s" id (Connection.get_domstr c) in
 		let perform_exn (request, response) =
+			write_access_log ~ty:request.Packet.ty ~tid ~con ~data:request.Packet.data;
 			let fct = function_of_type_simple_op request.Packet.ty in
 			let response' = input_handle_error ~cons ~doms ~fct ~con:c ~t:new_t ~req:request in
+			write_response_log ~ty:request.Packet.ty ~tid ~con ~response:response';
 			if not(Packet.response_equal response response') then raise Transaction_again in
 		finally
 		(fun () ->
@@ -451,12 +465,6 @@ let process_packet ~store ~cons ~doms ~con ~req =
 		error "process packet: %s" (Printexc.to_string exn);
 		Connection.send_error con tid rid "EIO"
 
-let write_access_log ~ty ~tid ~con ~data =
-	Logging.xb_op ~ty ~tid ~con:(Connection.get_domstr con) data
-
-let write_answer_log ~ty ~tid ~con ~data =
-	Logging.xb_answer ~ty ~tid ~con:(Connection.get_domstr con) data
-
 let do_input store cons doms con =
 	let newpacket =
 		try
@@ -483,7 +491,7 @@ let do_input store cons doms con =
 		         (Connection.get_domstr con) tid
 		         (Xenbus.Xb.Op.to_string ty) (sanitize_data data); *)
 		process_packet ~store ~cons ~doms ~con ~req;
-		write_access_log ~ty ~tid ~con ~data;
+		write_access_log ~ty ~tid ~con:(Connection.get_domstr con) ~data;
 		Connection.incr_ops con;
 	)
 
@@ -496,7 +504,7 @@ let do_output store cons doms con =
 			   info "[%s] <- %s \"%s\""
 			         (Connection.get_domstr con)
 			         (Xenbus.Xb.Op.to_string ty) (sanitize_data data);*)
-			write_answer_log ~ty ~tid ~con ~data;
+			write_answer_log ~ty ~tid ~con:(Connection.get_domstr con) ~data;
 		);
 		try
 			ignore (Connection.do_output con)
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0/7] oxenstored: improve transaction conflict handling

[Konrad Rzeszutek Wilk]

On Thu, Mar 17, 2016 at 05:51:08PM +0000, Jonathan Davies wrote:
> This patch series makes a substantial improvement to oxenstored's transaction
> handling.
> 
> The original design of oxenstored assumed that a transaction would only ever
> span a small subtree. In practice this is rarely the case and means that other
> xenstore traffic overlapping with a transaction can cause it to fail with
> EAGAIN. This leads to significant performance problems with toolstack operations
> when there is a certain level of xenstore traffic coming from other sources.
> 
> Instead, we observe that, although the transaction may span a large subtree, it
> is very unlikely to read or write the same keys as other xenstore traffic. This
> observation leads to an improved transaction conflict-handling algorithm in
> which the transaction is replayed to check it is serializable. This approach is
> superior in performance.
> 
> The sixth patch is the main one that changes the way transaction conflicts are
> handled. The first five patches prepare the way by performing some refactoring
> and addition of infrastructure that allows for transactions to be replayed. The
> seventh patch adds some extra logging.

All the patches have quite the Reviewed-by list already so I would think
these can go in now?

Thought oddly I didn't see any discussion on xen-devel for this?

> 
> Jonathan Davies (7):
>   oxenstored: refactor putting response on wire
>   oxenstored: remove some unused parameters
>   oxenstored: refactor request processing
>   oxenstored: keep track of each transaction's operations
>   oxenstored: move functions that process simple operations
>   oxenstored: replay transaction upon conflict
>   oxenstored: log request and response during transaction replay
> 
>  tools/ocaml/xenstored/Makefile        |    1 +
>  tools/ocaml/xenstored/connection.ml   |    5 +-
>  tools/ocaml/xenstored/define.ml       |    1 +
>  tools/ocaml/xenstored/oxenstored.conf |    1 +
>  tools/ocaml/xenstored/process.ml      |  344 +++++++++++++++++++++------------
>  tools/ocaml/xenstored/transaction.ml  |   21 +-
>  tools/ocaml/xenstored/xenstored.ml    |    1 +
>  7 files changed, 239 insertions(+), 135 deletions(-)
> 
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0/7] oxenstored: improve transaction conflict handling

[Jonathan Davies]

On Fri, Mar 18, 2016 at 10:33:35AM -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, Mar 17, 2016 at 05:51:08PM +0000, Jonathan Davies wrote:
> > This patch series makes a substantial improvement to oxenstored's transaction
> > handling.
> > 
> > The original design of oxenstored assumed that a transaction would only ever
> > span a small subtree. In practice this is rarely the case and means that other
> > xenstore traffic overlapping with a transaction can cause it to fail with
> > EAGAIN. This leads to significant performance problems with toolstack operations
> > when there is a certain level of xenstore traffic coming from other sources.
> > 
> > Instead, we observe that, although the transaction may span a large subtree, it
> > is very unlikely to read or write the same keys as other xenstore traffic. This
> > observation leads to an improved transaction conflict-handling algorithm in
> > which the transaction is replayed to check it is serializable. This approach is
> > superior in performance.
> > 
> > The sixth patch is the main one that changes the way transaction conflicts are
> > handled. The first five patches prepare the way by performing some refactoring
> > and addition of infrastructure that allows for transactions to be replayed. The
> > seventh patch adds some extra logging.
> 
> All the patches have quite the Reviewed-by list already so I would think
> these can go in now?
> 
> Thought oddly I didn't see any discussion on xen-devel for this?

Indeed; we did a lot of testing internally before these patches were posted.

Jonathan

> > Jonathan Davies (7):
> >   oxenstored: refactor putting response on wire
> >   oxenstored: remove some unused parameters
> >   oxenstored: refactor request processing
> >   oxenstored: keep track of each transaction's operations
> >   oxenstored: move functions that process simple operations
> >   oxenstored: replay transaction upon conflict
> >   oxenstored: log request and response during transaction replay
> > 
> >  tools/ocaml/xenstored/Makefile        |    1 +
> >  tools/ocaml/xenstored/connection.ml   |    5 +-
> >  tools/ocaml/xenstored/define.ml       |    1 +
> >  tools/ocaml/xenstored/oxenstored.conf |    1 +
> >  tools/ocaml/xenstored/process.ml      |  344 +++++++++++++++++++++------------
> >  tools/ocaml/xenstored/transaction.ml  |   21 +-
> >  tools/ocaml/xenstored/xenstored.ml    |    1 +
> >  7 files changed, 239 insertions(+), 135 deletions(-)
> > 
> > -- 
> > 1.7.10.4
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0/7] oxenstored: improve transaction conflict handling

[Wei Liu]

On Fri, Mar 18, 2016 at 10:33:35AM -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, Mar 17, 2016 at 05:51:08PM +0000, Jonathan Davies wrote:
> > This patch series makes a substantial improvement to oxenstored's transaction
> > handling.
> > 
> > The original design of oxenstored assumed that a transaction would only ever
> > span a small subtree. In practice this is rarely the case and means that other
> > xenstore traffic overlapping with a transaction can cause it to fail with
> > EAGAIN. This leads to significant performance problems with toolstack operations
> > when there is a certain level of xenstore traffic coming from other sources.
> > 
> > Instead, we observe that, although the transaction may span a large subtree, it
> > is very unlikely to read or write the same keys as other xenstore traffic. This
> > observation leads to an improved transaction conflict-handling algorithm in
> > which the transaction is replayed to check it is serializable. This approach is
> > superior in performance.
> > 
> > The sixth patch is the main one that changes the way transaction conflicts are
> > handled. The first five patches prepare the way by performing some refactoring
> > and addition of infrastructure that allows for transactions to be replayed. The
> > seventh patch adds some extra logging.
> 
> All the patches have quite the Reviewed-by list already so I would think
> these can go in now?
> 

We need an ack from Dave.

But, FWIW, the result is impressive, and the changes are only internal
to oxenstored, so I think it's pretty safe interface-wise or
protocol-wise.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0/7] oxenstored: improve transaction conflict handling

[David Scott]

> On 18 Mar 2016, at 16:36, Wei Liu <wei.liu2@citrix.com> wrote:
> 
> On Fri, Mar 18, 2016 at 10:33:35AM -0400, Konrad Rzeszutek Wilk wrote:
>> On Thu, Mar 17, 2016 at 05:51:08PM +0000, Jonathan Davies wrote:
>>> This patch series makes a substantial improvement to oxenstored's transaction
>>> handling.
>>> 
>>> The original design of oxenstored assumed that a transaction would only ever
>>> span a small subtree. In practice this is rarely the case and means that other
>>> xenstore traffic overlapping with a transaction can cause it to fail with
>>> EAGAIN. This leads to significant performance problems with toolstack operations
>>> when there is a certain level of xenstore traffic coming from other sources.
>>> 
>>> Instead, we observe that, although the transaction may span a large subtree, it
>>> is very unlikely to read or write the same keys as other xenstore traffic. This
>>> observation leads to an improved transaction conflict-handling algorithm in
>>> which the transaction is replayed to check it is serializable. This approach is
>>> superior in performance.
>>> 
>>> The sixth patch is the main one that changes the way transaction conflicts are
>>> handled. The first five patches prepare the way by performing some refactoring
>>> and addition of infrastructure that allows for transactions to be replayed. The
>>> seventh patch adds some extra logging.
>> 
>> All the patches have quite the Reviewed-by list already so I would think
>> these can go in now?
>> 
> 
> We need an ack from Dave.

Thanks for the reminder!

Acked-by: David Scott <dave@recoil.org>


> But, FWIW, the result is impressive, and the changes are only internal
> to oxenstored, so I think it's pretty safe interface-wise or
> protocol-wise.

Indeed — thanks, Jonathan, for putting this together and thanks to Andrew, Jon and Euan for the review! I’ve wanted to see this work for ages, so I’m really pleased that it’s finally happened :-)

Cheers,
Dave
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel