Skip to contentSkip to conversation

oxenstored fixes -- fixes recent pvops kernel hang

PATCH Superseded Ian Campbell 5 participants 16 messages Dec 9, 2011 – Dec 13, 2011

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 oxenstored fixes -- fixes recent pvops kernel hang

Currently PVHVM Linux guests after ddacf5ef684a "xen/pv-on-hvm kexec:
add xs_reset_watches to shutdown watches from old kernel" hang when run against oxenstored because it does not handle the unknown XS_RESET_WATCHES operation and does not reply.

The symptom of this issue is a hang during boot at this point:
cpu 1 spinlock event irq 70
CPU 1 irqstacks, hard=dec94000 soft=dec96000
Booting Node 0, Processors #1
smpboot cpu 1: start_ip = 99000
Initializing CPU#1
installing Xen timer for CPU 1
Brought up 2 CPUs
Total of 2 processors activated (9625.99 BogoMIPS).
NET: Registered protocol family 16
<HANG>

This series makes oxenstored handle unknown operations by returning an error indicating that the operation is unknown. I have not actually implemented support for XS_RESET_WATCHES.

Two other small fixlets are included.

Lastly I include a patch which I've been using for some time to enable the use of oxenstored in preference to C xenstored when available.

#1 lore raw

PATCH 1/1 oxenstored: Remove support for PQ defined operations

# HG changeset patch
# User Ian Campbell <[email protected]> # Date 1323447576 0
# Node ID beaf8eb93c36062b395bab3a30e3cf9d1f335a73 # Parent 3429972bbc6a1b998b13ba38f449fbb8251b3717
oxenstored: Remove support for PQ defined operations.

It is unused.

tools/ocaml/libs/xb/op.ml +0 -13
diff -r 3429972bbc6a -r beaf8eb93c36 tools/ocaml/libs/xb/op.ml
@@ -22,9 +22,6 @@ type operation = Debug | Directory | Rea
                  Resume | Set_target
                | Restrict 
 
-(* There are two sets of XB operations: the one coming from open-source and *)
-(* the one coming from our private patch queue. These operations            *)
-(* in two differents arrays for make easier the forward compatibility       *)
 let operation_c_mapping =
 	[| Debug; Directory; Read; Getperms;
            Watch; Unwatch; Transaction_start;
@@ -34,12 +31,6 @@ let operation_c_mapping =
            Resume; Set_target; Restrict |]
 let size = Array.length operation_c_mapping
 
-(* [offset_pq] has to be the same as in <xen/io/xs_wire.h> *)
-let offset_pq = size
-let operation_c_mapping_pq =
-	[| |]
-let size_pq = Array.length operation_c_mapping_pq
-
 let array_search el a =
 	let len = Array.length a in
 	let rec search i =
@@ -50,14 +41,10 @@ let array_search el a =
 let of_cval i =
 	if i >= 0 && i < size
 	then operation_c_mapping.(i)
-	else if i >= offset_pq && i < offset_pq + size_pq
-	then operation_c_mapping_pq.(i-offset_pq)
 	else raise Not_found
 
 let to_cval op =
-	try
 	array_search op operation_c_mapping
-	with _ -> offset_pq + array_search op operation_c_mapping_pq
 
 let to_string ty =
 	match ty with
tools/ocaml/libs/xb/xb.mli +0 -3
diff -r 3429972bbc6a -r beaf8eb93c36 tools/ocaml/libs/xb/xb.mli
@@ -25,9 +25,6 @@ module Op :
       | Restrict
     val operation_c_mapping : operation array
     val size : int
-    val offset_pq : int
-    val operation_c_mapping_pq : 'a array
-    val size_pq : int
     val array_search : 'a -> 'a array -> int
     val of_cval : int -> operation
     val to_cval : operation -> int
#2 lore raw

PATCH 1/1 oxenstored: log Errors and Warnings by default

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]> # Date 1323447576 0
# Node ID 18c0807a0504791bcb6e083866def9c4862a119a # Parent beaf8eb93c36062b395bab3a30e3cf9d1f335a73
oxenstored: log Errors and Warnings by default.

tools/ocaml/xenstored/logging.ml +1 -1
diff -r beaf8eb93c36 -r 18c0807a0504 tools/ocaml/xenstored/logging.ml
@@ -105,7 +105,7 @@ let string_of_date () =
 		(int_of_float (1000.0 *. msec))
 
 let xenstored_log_file = ref "/var/log/xenstored.log"
-let xenstored_log_level = ref Null
+let xenstored_log_level = ref Warn
 let xenstored_log_nb_files = ref 10
 let xenstored_log_nb_lines = ref 13215
 let xenstored_log_nb_chars = ref (-1)
#3 lore raw

PATCH 1/1 oxenstored: handle unknown operations by returning an error to the client

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1323448556 0
# Node ID 74f94e15bfe1dad412d0342aeabdbd895657f54f
# Parent 18c0807a0504791bcb6e083866def9c4862a119a
oxenstored: handle unknown operations by returning an error to the client

Previous an unknown operation would be decoded as a Not_found exception which would bubble all the way up to the try ... with surrounding the call to main_loop where it would be logged and ignored.

This would leave the guest hanging waiting for a response to the invalid request.

Instead introduce a specific "Invalid" operation. Higher level functionality, such as Process.process_packet, already handles operations which are not understood with an error reply due to the final wildcard entry in Process.function_of_type but explicitly handle Invalid this way to make it clear what is going on.

tools/ocaml/libs/xb/op.ml +3 -3
diff -r 18c0807a0504 -r 74f94e15bfe1 tools/ocaml/libs/xb/op.ml
@@ -19,8 +19,7 @@ type operation = Debug | Directory | Rea
                  Transaction_end | Introduce | Release |
                  Getdomainpath | Write | Mkdir | Rm |
                  Setperms | Watchevent | Error | Isintroduced |
-                 Resume | Set_target
-               | Restrict 
+                 Resume | Set_target | Restrict | Invalid
 
 let operation_c_mapping =
 	[| Debug; Directory; Read; Getperms;
@@ -41,7 +40,7 @@ let array_search el a =
 let of_cval i =
 	if i >= 0 && i < size
 	then operation_c_mapping.(i)
-	else raise Not_found
+	else Invalid
 
 let to_cval op =
 	array_search op operation_c_mapping
@@ -69,3 +68,4 @@ let to_string ty =
 	| Resume		-> "RESUME"
 	| Set_target		-> "SET_TARGET"
 	| Restrict		-> "RESTRICT"
+	| Invalid		-> "INVALID"
tools/ocaml/libs/xb/xb.mli +1 -0
diff -r 18c0807a0504 -r 74f94e15bfe1 tools/ocaml/libs/xb/xb.mli
@@ -23,6 +23,7 @@ module Op :
       | Resume
       | Set_target
       | Restrict
+      | Invalid (* Not a valid wire operation *)
     val operation_c_mapping : operation array
     val size : int
     val array_search : 'a -> 'a array -> int
tools/ocaml/xenstored/process.ml +2 -1
diff -r 18c0807a0504 -r 74f94e15bfe1 tools/ocaml/xenstored/process.ml
@@ -324,7 +324,8 @@ let function_of_type ty =
 	| 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
-	| _                       -> reply_ack do_error
+	| 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 reply_error e =
tools/ocaml/xenstored/xenstored.ml +1 -3
diff -r 18c0807a0504 -r 74f94e15bfe1 tools/ocaml/xenstored/xenstored.ml
@@ -43,9 +43,7 @@ let process_connection_fds store cons do
 			debug "closing socket connection"
 		in
 	let process_fdset_with fds fct =
-		List.iter (fun fd ->
-		           try try_fct fct (Connections.find cons fd)
-		           with Not_found -> ()) fds
+		List.iter (fun fd -> try_fct fct (Connections.find cons fd)) fds
 	in
 	process_fdset_with rset Process.do_input;
 	process_fdset_with wset Process.do_output
#4 lore raw

Re: [PATCH 3 of 4] oxenstored: handle unknown operations by returning an error to the client

Ian Campbell writes ("[Xen-devel] [PATCH 3 of 4] oxenstored: handle unknown operations by returning an error to the client"):

Quoted 1 line

oxenstored: handle unknown operations by returning an error to the client

This introduces a new warning:

File "logging.ml", line 157, characters 14-1067:
Warning P: this pattern-matching is not exhaustive. Here is an example of a value that is not matched:
Invalid

I have applied 1/4 and 2/4.

Ian.

#5 lore raw

Re: [PATCH 3 of 4] oxenstored: handle unknown operations by returning an error to the client

Quoted 9 lines · Ian Jackson

Ian Campbell writes ("[Xen-devel] [PATCH 3 of 4] oxenstored: handle unknown operations by returning an error to the client"):

Quoted 1 line

oxenstored: handle unknown operations by returning an error to the client

This introduces a new warning:

File "logging.ml", line 157, characters 14-1067:
Warning P: this pattern-matching is not exhaustive. Here is an example of a value that is not matched:
Invalid

Sorry, will fix and resend.

Quoted 1 line

I have applied 1/4 and 2/4.

Did you see "5/4" and "6/4"? In any case I shall include them in my resend.

Ian.

#6 lore raw

PATCH 1/1 Linux/xencommons: Use oxenstored by default when available

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1323448636 0
# Node ID 3b7ac401f144206c30440fbb41c74b13fa20b8cb # Parent 74f94e15bfe1dad412d0342aeabdbd895657f54f Linux/xencommons: Use oxenstored by default when available

oxenstored is an ocaml implementation of the xenstore daemon. It is faster, more scalable and more reliable than the C xenstored.

In particular the transaction model in oxenstored does not involve taking a complete copy of the database and aborting on any (even non-conflicting) other change.

There is a paper on the design and implementation of oxenstored at
http://gazagnaire.org/pub/GH09.pdf which includes a performance evaluation and comparison with the C daemon etc.

tools/hotplug/Linux/init.d/sysconfig.xencommons +3 -0
diff -r 74f94e15bfe1 -r 3b7ac401f144 tools/hotplug/Linux/init.d/sysconfig.xencommons
@@ -1,6 +1,9 @@
 # Log xenconsoled messages (cf xl dmesg)
 XENCONSOLED_TRACE=guest
 
+# Select xenstored implementation
+#XENSTORED=[oxenstored|xenstored]
+
 # Log xenstored messages
 #XENSTORED_TRACE=[yes|on|1]
 
tools/hotplug/Linux/init.d/xencommons +13 -2
diff -r 74f94e15bfe1 -r 3b7ac401f144 tools/hotplug/Linux/init.d/xencommons
@@ -70,8 +70,19 @@ do_start () {
 		rm -f "$XENSTORED_ROOTDIR"/tdb* &>/dev/null
 		test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
 
-		echo -n Starting xenstored...
-		xenstored --pid-file=/var/run/xenstored.pid $XENSTORED_ARGS
+		if [ -n "$XENSTORED" ] ; then
+		    echo -n Starting $XENSTORED...
+		    $XENSTORED --pid-file /var/run/xenstored.pid $XENSTORED_ARGS
+		elif [ -x /usr/sbin/oxenstored ] ; then
+		    echo -n Starting oxenstored...
+		    /usr/sbin/oxenstored --pid-file /var/run/xenstored.pid $XENSTORED_ARGS
+		elif [ -x /usr/sbin/xenstored ] ; then
+		    echo -n Starting C xenstored...
+		    /usr/sbin/xenstored --pid-file /var/run/xenstored.pid $XENSTORED_ARGS
+		else
+		    echo "No xenstored found"
+		    exit 1
+		fi
 
 		# Wait for xenstored to actually come up, timing out after 30 seconds
                 while [ $time -lt $timeout ] && ! `xenstore-read -s / >/dev/null 2>&1` ; do
Acked-by on v1 1/1
#7 lore raw

Re: [PATCH 4 of 4] Linux/xencommons: Use oxenstored by default when available

Quoted 6 lines · Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1323448636 0
# Node ID 3b7ac401f144206c30440fbb41c74b13fa20b8cb # Parent 74f94e15bfe1dad412d0342aeabdbd895657f54f Linux/xencommons: Use oxenstored by default when available

Assuming this has been tested:

#8 lore raw

Re: [PATCH 4 of 4] Linux/xencommons: Use oxenstored by default when available

Quoted 10 lines · Olaf Hering
Quoted 6 lines · Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1323448636 0
# Node ID 3b7ac401f144206c30440fbb41c74b13fa20b8cb # Parent 74f94e15bfe1dad412d0342aeabdbd895657f54f Linux/xencommons: Use oxenstored by default when available

Assuming this has been tested:

I've been running with it for quite a while. Perhaps more importantly XenServer has been using it since, well, since I can't remember when but it was several years ago when they switched and they pound it pretty hard in their testing. Harder than C xenstored could cope with which is why this one got written...

Another thing I forgot to mention is that by default the database is non-persistent and held in RAM, so no need for hacks like putting the db file on a tmpfs.

Ian.

#9 lore raw

Re: [PATCH 4 of 4] Linux/xencommons: Use oxenstored by default when available

Quoted 10 lines · Ian Campbell
Quoted 9 lines · Olaf Hering
Quoted 6 lines · Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1323448636 0
# Node ID 3b7ac401f144206c30440fbb41c74b13fa20b8cb # Parent 74f94e15bfe1dad412d0342aeabdbd895657f54f Linux/xencommons: Use oxenstored by default when available

Quoted 7 lines
Quoted 1 line

Assuming this has been tested:

I've been running with it for quite a while. Perhaps more importantly XenServer has been using it since, well, since I can't remember when but it was several years ago when they switched and they pound it pretty hard in their testing. Harder than C xenstored could cope with which is why this one got written...

However is our version up in sync with theirs I wonder... Are we missing any important bug fix?

#10 lore raw

Re: [PATCH 4 of 4] Linux/xencommons: Use oxenstored by default when available

Quoted 13 lines · Stefano Stabellini
Quoted 12 lines · Ian Campbell
Quoted 10 lines · Olaf Hering
Quoted 6 lines · Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1323448636 0
# Node ID 3b7ac401f144206c30440fbb41c74b13fa20b8cb # Parent 74f94e15bfe1dad412d0342aeabdbd895657f54f Linux/xencommons: Use oxenstored by default when available

Assuming this has been tested:

Quoted 8 lines
Quoted 5 lines

I've been running with it for quite a while. Perhaps more importantly XenServer has been using it since, well, since I can't remember when but it was several years ago when they switched and they pound it pretty hard in their testing. Harder than C xenstored could cope with which is why this one got written...

However is our version up in sync with theirs I wonder... Are we missing any important bug fix?

XenServer switched to the one in the xen tree in March by the looks of things, at least that's when the copy in xen-api.git was removed.

There is nothing of interest in xen-api.git's copy from the point it was imported into xen-unstable until now, if anything xen-unstable has been the one getting fixes.

Ian.

#11 lore raw

Re: [PATCH 4 of 4] Linux/xencommons: Use oxenstored by default when available

In reply to Ian Campbell

2011/12/9 Ian Campbell <[email protected]>:

Quoted 9 lines

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1323448636 0
# Node ID 3b7ac401f144206c30440fbb41c74b13fa20b8cb # Parent  74f94e15bfe1dad412d0342aeabdbd895657f54f Linux/xencommons: Use oxenstored by default when available

oxenstored is an ocaml implementation of the xenstore daemon. It is faster, more scalable and more reliable than the C xenstored.

This means that C xenstore is going the way of the Dodo? Or are we going to maintain both implementations (ocalm and C)?

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#12 lore raw

Re: [PATCH 4 of 4] Linux/xencommons: Use oxenstored by default when available

Quoted 8 lines · Roger Pau Monné

2011/12/9 Ian Campbell <[email protected]>:

Quoted 7 lines

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1323448636 0
# Node ID 3b7ac401f144206c30440fbb41c74b13fa20b8cb # Parent 74f94e15bfe1dad412d0342aeabdbd895657f54f Linux/xencommons: Use oxenstored by default when available

Quoted 5 lines
Quoted 2 lines

oxenstored is an ocaml implementation of the xenstore daemon. It is faster, more scalable and more reliable than the C xenstored.

This means that C xenstore is going the way of the Dodo? Or are we going to maintain both implementations (ocalm and C)?

I honestly don't know.

They are both reasonably low maintenance and rarely change or have bugs reported. We don't actually see many bug reports about C xenstored although it's short comings only tend to become apparent at scale and I suspect lots of people use workarounds like nuking the tdb on boot and/or putting it on a tmpfs.

On the other hand having two code bases providing the same service has its own downsides.

I expect our (or at least my) first response to a non-obvious bug report in C xenstored would be "have you tried oxenstored?".

FWIW my intention (when I eventually get to it and resurrect those patches) is for stub-xenstored to be an oxenstored only thing.

Ian.

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#13 lore raw
In reply to Ian Campbell
Quoted 1 line · Ian Campbell

Two other small fixlets are included.

Actually, that should have been four. I'll post the other two as separate replies to this mail. They will be:

oxenstored: install configuration file
oxenstored: Always log something at start of day (if logging enabled at all)

Ian.

#14 lore raw

PATCH 1/1 oxenstored: install configuration file

# HG changeset patch
# User Ian Campbell <[email protected]> # Date 1323449332 0
# Node ID 91415c125c676d9e87d9939bfecdd1a4a0b2edfa # Parent 3b7ac401f144206c30440fbb41c74b13fa20b8cb
oxenstored: install configuration file

First though:
- Move it to /etc/xen/oxenstored.conf.
- Use /var/run/xenstored.pid as default pid file
- Disable test-eagain "Randomly failed a transaction with EAGAIN. Used for testing Xs user". Doesn't sound fun by default...

tools/ocaml/xenstored/Makefile +2 -0
@@ -52,5 +52,7 @@ bins: $(PROGRAMS)
 install: all
 	$(INSTALL_DIR) $(DESTDIR)$(SBINDIR)
 	$(INSTALL_PROG) oxenstored $(DESTDIR)$(SBINDIR)
+	$(INSTALL_DIR) $(DESTDIR)$(XEN_CONFIG_DIR)
+	$(INSTALL_DATA) oxenstored.conf $(DESTDIR)$(XEN_CONFIG_DIR)
 
 include $(OCAML_TOPLEVEL)/Makefile.rules
tools/ocaml/xenstored/define.ml +1 -1
@@ -23,7 +23,7 @@ let xenstored_proc_port = "/proc/xen/xsd
 let xs_daemon_socket = "/var/run/xenstored/socket"
 let xs_daemon_socket_ro = "/var/run/xenstored/socket_ro"
 
-let default_config_dir = "/etc/xensource"
+let default_config_dir = "/etc/xen"
 
 let maxwatch = ref (50)
 let maxtransaction = ref (20)
tools/ocaml/xenstored/oxenstored.conf renamed from tools/ocaml/xenstored/xenstored.conf +2 -2
rename from tools/ocaml/xenstored/xenstored.conf
rename to tools/ocaml/xenstored/oxenstored.conf
@@ -1,10 +1,10 @@
 # default xenstored config
 
 # Where the pid file is stored
-pid-file = /var/run/xensource/xenstored.pid
+pid-file = /var/run/xenstored.pid
 
 # Randomly failed a transaction with EAGAIN. Used for testing Xs user
-test-eagain = true
+test-eagain = false
 
 # Activate transaction merge support
 merge-activate = true
tools/ocaml/xenstored/xenstored.ml +1 -1
@@ -71,7 +71,7 @@ let sighup_handler _ =
 let config_filename cf =
 	match cf.config_file with
 	| Some name -> name
-	| None      -> Define.default_config_dir ^ "/xenstored.conf"
+	| None      -> Define.default_config_dir ^ "/oxenstored.conf"
 
 let default_pidfile = "/var/run/xenstored.pid"
#15 lore raw

PATCH 1/1 oxenstored: Always log something at start of day (if logging enabled at all)

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1323450179 0
# Node ID c2fbf133c235c023769640fdb24d84462a6b36ec
# Parent 91415c125c676d9e87d9939bfecdd1a4a0b2edfa
oxenstored: Always log something at start of day (if logging enabled at all)

Otherwise at the default level we rarely log anything at all.

A completely empty log file is a good sign, but only if you know you are looking in the right place...

tools/ocaml/xenstored/logging.ml +3 -0
@@ -117,6 +117,9 @@ let init_xenstored_log () =
 			make_logger 
 				!xenstored_log_file !xenstored_log_nb_files !xenstored_log_nb_lines
 				!xenstored_log_nb_chars ignore in
+		let date = string_of_date() in
+		logger.write ("[%s|%5s|%s] Xen Storage Daemon, version %d.%d") date "" "startup"
+		  Define.xenstored_major Define.xenstored_minor;
 		xenstored_logger := Some logger
 
 let xenstored_logging level key (fmt: (_,_,_,_) format4) =
tools/ocaml/xenstored/xenstored.ml +0 -3
@@ -284,9 +284,6 @@ let _ =
 		Logging.init_access_log post_rotate
 	end;
 
-	info "Xen Storage Daemon, version %d.%d"
-	     Define.xenstored_major Define.xenstored_minor;
-
 	let spec_fds =
 		(match rw_sock with None -> [] | Some x -> [ x ]) @
 		(match ro_sock with None -> [] | Some x -> [ x ]) @