[PATCH v2] xentop: add support for qdisks

[PATCH v2] xentop: add support for qdisks

[Charles Arnold]

Now that Xen uses qdisks by default and qemu does not write out
statistics to sysfs this patch queries the QMP for disk statistics.

This patch depends on libyajl for parsing statistics returned from
QMP. The runtime requires libyajl 2.0.3 or newer for required bug
fixes in yajl_tree_parse().

No interfaces have been changed. It works within the existing
framework of libxenstat.

Signed-off-by: Charles Arnold <carnold@suse.com>

diff --git a/tools/xenstat/libxenstat/Makefile b/tools/xenstat/libxenstat/Makefile
index 07d39b1..6a69f7e 100644
--- a/tools/xenstat/libxenstat/Makefile
+++ b/tools/xenstat/libxenstat/Makefile
@@ -24,7 +24,7 @@ MINOR=0
 LIB=src/libxenstat.a
 SHLIB=src/libxenstat.so.$(MAJOR).$(MINOR)
 SHLIB_LINKS=src/libxenstat.so.$(MAJOR) src/libxenstat.so
-OBJECTS-y=src/xenstat.o
+OBJECTS-y=src/xenstat.o src/xenstat_qmp.o
 OBJECTS-$(CONFIG_Linux) += src/xenstat_linux.o
 OBJECTS-$(CONFIG_SunOS) += src/xenstat_solaris.o
 OBJECTS-$(CONFIG_NetBSD) += src/xenstat_netbsd.o
diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c
index 8072a90..f3847be 100644
--- a/tools/xenstat/libxenstat/src/xenstat.c
+++ b/tools/xenstat/libxenstat/src/xenstat.c
@@ -657,6 +657,24 @@ static void xenstat_uninit_xen_version(xenstat_handle * handle)
  * VBD functions
  */
 
+/* Save VBD information */
+xenstat_vbd *xenstat_save_vbd(xenstat_domain *domain, xenstat_vbd *vbd)
+{
+        if (domain->vbds == NULL) {
+                domain->num_vbds = 1;
+                domain->vbds = malloc(sizeof(xenstat_vbd));
+        } else {
+                domain->num_vbds++;
+                domain->vbds = realloc(domain->vbds,
+                                       domain->num_vbds *
+                                       sizeof(xenstat_vbd));
+        }
+        if (domain->vbds != NULL)
+                domain->vbds[domain->num_vbds - 1] = *vbd;
+
+        return domain->vbds;
+}
+
 /* Free VBD information */
 static void xenstat_free_vbds(xenstat_node * node)
 {
diff --git a/tools/xenstat/libxenstat/src/xenstat_linux.c b/tools/xenstat/libxenstat/src/xenstat_linux.c
index 7fdf70a..2cc9c7f 100644
--- a/tools/xenstat/libxenstat/src/xenstat_linux.c
+++ b/tools/xenstat/libxenstat/src/xenstat_linux.c
@@ -417,6 +417,9 @@ int xenstat_collect_vbds(xenstat_node * node)
 		}
 	}
 
+	/* Get qdisk statistics */
+	read_attributes_qdisk(node);
+
 	rewinddir(priv->sysfsvbd);
 
 	for(dp = readdir(priv->sysfsvbd); dp != NULL ;
@@ -477,18 +480,10 @@ int xenstat_collect_vbds(xenstat_node * node)
 			continue;
 		}
 
-		if (domain->vbds == NULL) {
-			domain->num_vbds = 1;
-			domain->vbds = malloc(sizeof(xenstat_vbd));
-		} else {
-			domain->num_vbds++;
-			domain->vbds = realloc(domain->vbds,
-					       domain->num_vbds *
-					       sizeof(xenstat_vbd));
-		}
-		if (domain->vbds == NULL)
+		if ((xenstat_save_vbd(domain, &vbd)) == NULL) {
+			perror("Allocation error");
 			return 0;
-		domain->vbds[domain->num_vbds - 1] = vbd;
+		}
 	}
 
 	return 1;	
diff --git a/tools/xenstat/libxenstat/src/xenstat_priv.h b/tools/xenstat/libxenstat/src/xenstat_priv.h
index 8490e23..74e0774 100644
--- a/tools/xenstat/libxenstat/src/xenstat_priv.h
+++ b/tools/xenstat/libxenstat/src/xenstat_priv.h
@@ -109,5 +109,7 @@ extern int xenstat_collect_networks(xenstat_node * node);
 extern void xenstat_uninit_networks(xenstat_handle * handle);
 extern int xenstat_collect_vbds(xenstat_node * node);
 extern void xenstat_uninit_vbds(xenstat_handle * handle);
+extern void read_attributes_qdisk(xenstat_node * node);
+extern xenstat_vbd *xenstat_save_vbd(xenstat_domain * domain, xenstat_vbd * vbd);
 
 #endif /* XENSTAT_PRIV_H */
diff --git a/tools/xenstat/xentop/Makefile b/tools/xenstat/xentop/Makefile
index e2665aa..5376fdc 100644
--- a/tools/xenstat/xentop/Makefile
+++ b/tools/xenstat/xentop/Makefile
@@ -19,7 +19,7 @@ all install xentop:
 else
 
 CFLAGS += -DGCC_PRINTF -Werror $(CFLAGS_libxenstat)
-LDLIBS += $(LDLIBS_libxenstat) $(CURSES_LIBS) $(SOCKET_LIBS) -lm
+LDLIBS += $(LDLIBS_libxenstat) $(CURSES_LIBS) $(SOCKET_LIBS) -lm -lyajl
 CFLAGS += -DHOST_$(XEN_OS)
 
 # Include configure output (config.h) to headers search path
diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
new file mode 100644
index 0000000..aac4637
--- /dev/null
+++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
@@ -0,0 +1,387 @@
+/* libxenstat: statistics-collection library for Xen
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ */
+
+#include <fcntl.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/poll.h>
+#include <sys/un.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include "yajl/yajl_tree.h"
+
+#include <xenctrl.h>
+
+#include "xenstat_priv.h"
+
+static unsigned char *qmp_query(int, char *);
+
+enum query_blockstats {
+    QMP_STATS_RETURN  = 0,
+    QMP_STATS_DEVICE  = 1,
+    QMP_STATS         = 2,
+    QMP_RD_BYTES      = 3,
+    QMP_WR_BYTES      = 4,
+    QMP_RD_OPERATIONS = 5,
+    QMP_WR_OPERATIONS = 6,
+};
+
+enum query_block {
+    QMP_BLOCK_RETURN  = 0,
+    QMP_BLOCK_DEVICE  = 1,
+    QMP_INSERTED      = 2,
+    QMP_FILE          = 3,
+};
+
+
+/* Given the qmp device name, get the image filename associated with it */
+static char *qmp_get_block_image(xenstat_node *node, char *qmp_devname, int qfd)
+{
+	char errbuf[1024], *tmp, *file = NULL;
+	char *query_block_cmd = "{ \"execute\": \"query-block\" }";
+	static const char *const qblock[] = {
+		[ QMP_BLOCK_RETURN  ] = "return",
+		[ QMP_BLOCK_DEVICE  ] = "device",
+		[ QMP_INSERTED      ] = "inserted",
+		[ QMP_FILE          ] = "file",
+	};
+	const char *ptr[] = {0, 0};
+	unsigned char *qmp_stats;
+	yajl_val info, ret_obj, dev_obj, n;
+	int i;
+
+	if ((qmp_stats = qmp_query(qfd, query_block_cmd)) == NULL)
+		return NULL;
+
+	/* Use libyajl version 2.1.x or newer for the tree parser feature with bug fixes */
+	if ((info = yajl_tree_parse((char *)qmp_stats, errbuf, sizeof(errbuf))) == NULL) {
+		free(qmp_stats);
+		return NULL;
+	}
+
+	ptr[0] = qblock[QMP_BLOCK_RETURN]; /* "return" */
+	if ((ret_obj = yajl_tree_get(info, ptr, yajl_t_array)) == NULL)
+		goto done;
+
+	for (i=0; i<YAJL_GET_ARRAY(ret_obj)->len; i++) {
+		n = YAJL_GET_ARRAY(ret_obj)->values[i];
+
+		ptr[0] = qblock[QMP_BLOCK_DEVICE]; /* "device" */
+		if ((dev_obj = yajl_tree_get(n, ptr, yajl_t_any)) != NULL) {
+			tmp = YAJL_GET_STRING(dev_obj);
+			if (strcmp(qmp_devname, tmp))
+				continue;
+		}
+		else
+			continue;
+
+		ptr[0] = qblock[QMP_INSERTED]; /* "inserted" */
+		n = yajl_tree_get(n, ptr, yajl_t_any);
+		if (n) {
+			ptr[0] = qblock[QMP_FILE]; /* "file" */
+			n = yajl_tree_get(n, ptr, yajl_t_any);
+			if (n && YAJL_IS_STRING(n)) {
+				tmp = YAJL_GET_STRING(n);
+				file = malloc(strlen(tmp)+1);
+				if (file != NULL)
+					strcpy(file, tmp);
+				goto done;
+			}
+		}
+	}
+done:
+	yajl_tree_free(info);
+	return file;
+}
+
+
+/* Given a QMP device name, find the associated xenstore qdisk device id */
+static void get_xs_devid_from_qmp_devname(xenstat_node * node, unsigned int domid, char *qmp_devname,
+	unsigned int *dev, unsigned int *sector_size, int qfd)
+{
+	char **dev_ids, *tmp, *ptr, *image, path[80];
+	unsigned int num_dev_ids;
+	int i, devid;
+
+	/* Get all the qdisk dev IDs associated with the this VM */
+	snprintf(path, sizeof(path),"/local/domain/0/backend/qdisk/%i", domid);
+	dev_ids = xs_directory(node->handle->xshandle, XBT_NULL, path, &num_dev_ids);
+	if (dev_ids == NULL) {
+		return;
+	}
+
+	/* Get the filename of the image associated with this QMP device */
+	image = qmp_get_block_image(node, qmp_devname, qfd);
+	if (image == NULL) {
+		free(dev_ids);
+		return;
+	}
+
+	/* Look for a matching image in xenstore */
+	for (i=0; i<num_dev_ids; i++) {
+		devid = atoi(dev_ids[i]);
+		/* Get the xenstore name of the image */
+		snprintf(path, sizeof(path),"/local/domain/0/backend/qdisk/%i/%i/params", domid, devid);
+		if ((ptr = xs_read(node->handle->xshandle, XBT_NULL, path, NULL)) == NULL)
+			continue;
+
+		/* Get to actual path in string */
+		if ((tmp = strchr(ptr, '/')) == NULL)
+			tmp = ptr;
+		if (!strcmp(tmp,image)) {
+			*dev = devid;
+			free(ptr);
+
+			/* Get the xenstore sector size of the image while we're here */
+			snprintf(path, sizeof(path),"/local/domain/0/backend/qdisk/%i/%i/sector-size", domid, devid);
+			if ((ptr = xs_read(node->handle->xshandle, XBT_NULL, path, NULL)) != NULL) {
+				*sector_size = atoi((char *)ptr);
+				free(ptr);
+			}
+			break;
+		}
+		free(ptr);
+	}
+
+	free(image);
+	free(dev_ids);
+}
+
+/* Parse the stats buffer which contains I/O data for all the disks belonging to domid */
+static void qmp_parse_stats(xenstat_node *node, unsigned int domid, unsigned char *stats_buf, int qfd)
+{
+	char *qmp_devname, errbuf[1024];
+	static const char *const qstats[] = {
+		[ QMP_STATS_RETURN  ] = "return",
+		[ QMP_STATS_DEVICE  ] = "device",
+		[ QMP_STATS         ] = "stats",
+		[ QMP_RD_BYTES      ] = "rd_bytes",
+		[ QMP_WR_BYTES      ] = "wr_bytes",
+		[ QMP_RD_OPERATIONS ] = "rd_operations",
+		[ QMP_WR_OPERATIONS ] = "wr_operations",
+	};
+	const char *ptr[] = {0, 0};
+	yajl_val info, ret_obj, stats_obj, n;
+	xenstat_vbd vbd;
+	xenstat_domain *domain;
+	unsigned int sector_size = 512;
+	int i, j;
+
+	/* Use libyajl version 2.0.3 or newer for the tree parser feature */
+	if ((info = yajl_tree_parse((char *)stats_buf, errbuf, sizeof(errbuf))) == NULL)
+		return;
+
+	ptr[0] = qstats[QMP_STATS_RETURN]; /* "return" */
+	if ((ret_obj = yajl_tree_get(info, ptr, yajl_t_array)) == NULL)
+		goto done;
+
+	/* Array of devices */
+	for (i=0; i<YAJL_GET_ARRAY(ret_obj)->len; i++) {
+		memset(&vbd, 0, sizeof(xenstat_vbd));
+		qmp_devname = NULL;
+		stats_obj = YAJL_GET_ARRAY(ret_obj)->values[i];
+
+		ptr[0] = qstats[QMP_STATS_DEVICE]; /* "device" */
+		if ((n = yajl_tree_get(stats_obj, ptr, yajl_t_any)) != NULL)
+			qmp_devname = YAJL_GET_STRING(n);
+
+		ptr[0] = qstats[QMP_STATS]; /* "stats" */
+		stats_obj = yajl_tree_get(stats_obj, ptr, yajl_t_object);
+		if (stats_obj && YAJL_IS_OBJECT(stats_obj)) {
+			for (j=3; j<7; j++) {
+				ptr[0] = qstats[j];
+				n = yajl_tree_get(stats_obj, ptr, yajl_t_number);
+				if (n && YAJL_IS_NUMBER(n)) {
+					switch(j) {
+					case QMP_RD_BYTES: /* "rd_bytes" */
+						vbd.rd_sects = YAJL_GET_INTEGER(n) / sector_size;
+						break;
+					case QMP_WR_BYTES: /* "wr_bytes" */
+						vbd.wr_sects = YAJL_GET_INTEGER(n) / sector_size;
+						break;
+					case QMP_RD_OPERATIONS: /* "rd_operations" */
+						vbd.rd_reqs = YAJL_GET_INTEGER(n);
+						break;
+					case QMP_WR_OPERATIONS: /* "wr_operations" */
+						vbd.wr_reqs = YAJL_GET_INTEGER(n);
+						break;
+					}
+				}
+			}
+			/* With the QMP device name, lookup the xenstore qdisk device ID and set vdb.dev */
+			if (qmp_devname)
+				get_xs_devid_from_qmp_devname(node, domid, qmp_devname, &vbd.dev, &sector_size, qfd);
+			if ((domain = xenstat_node_domain(node, domid)) == NULL)
+				continue;
+			if ((xenstat_save_vbd(domain, &vbd)) == NULL)
+				goto done;
+		}
+	}
+done:
+	yajl_tree_free(info);
+}
+
+/* Write a command via the QMP */
+static size_t qmp_write(int qfd, char *cmd, size_t cmd_len)
+{
+	size_t pos = 0;
+	ssize_t res;
+
+	while (cmd_len > pos) {
+		res = write(qfd, cmd + pos, cmd_len - pos);
+		switch (res) {
+		case -1:
+			if (errno == EINTR || errno == EAGAIN)
+				continue;
+			return 0;
+		case 0:
+			errno = EPIPE;
+			return pos;
+		default:
+			pos += (size_t)res;
+		}
+	}
+	return pos;
+}
+
+/* Read the data sent in response to a QMP execute query. Returns 1 for success */
+static int qmp_read(int qfd, unsigned char **qstats)
+{
+	unsigned char buf[1024], *ptr = NULL;
+	struct pollfd pfd[2];
+	int n, qsize = 0;
+
+	pfd[0].fd = qfd;
+	pfd[0].events = POLLIN;
+	while ((n = poll(pfd, POLLIN, 10)) > 0) {
+		if (pfd[0].revents & POLLIN) {
+			if ((n = read(qfd, buf, sizeof(buf))) < 0) {
+				return 0;
+			}
+			if (ptr == NULL)
+				ptr = malloc(n+1);
+			else
+				ptr = realloc(ptr, qsize+n+1);
+			if (ptr == NULL)
+				return 0;
+			memcpy(&ptr[qsize], buf, n);
+			qsize += n;
+			ptr[qsize] = 0;
+			*qstats = ptr;
+		}
+	}
+	return 1;
+}
+
+/* With the given cmd, query QMP for requested data. Returns allocated buffer containing data or NULL */
+static unsigned char *qmp_query(int qfd, char *cmd)
+{
+	unsigned char *qstats = NULL;
+	int n;
+
+	n = strlen(cmd);
+	if (qmp_write(qfd, cmd, n) != n)
+		return NULL;
+	if (!qmp_read(qfd, &qstats))
+		return NULL;
+	return qstats;
+}
+
+/* Returns a socket connected to the QMP socket. Returns -1 on failure. */
+static int qmp_connect(char *path)
+{
+	struct sockaddr_un sun;
+	int s;
+
+	if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0)
+		return -1;
+	(void)fcntl(s, F_SETFD, 1);
+
+	memset(&sun, 0, sizeof(struct sockaddr_un));
+	sun.sun_family = AF_UNIX;
+
+	if (strlen(path) >= sizeof(sun.sun_path)) {
+		close(s);
+		return -1;
+	}
+
+	strcpy(sun.sun_path, path);
+	if (connect(s, (struct sockaddr *)&sun, SUN_LEN(&sun)) < 0) {
+		close(s);
+		return -1;
+	}
+
+	return s;
+}
+
+/* Get all the active domains */
+static xc_domaininfo_t *get_domain_ids(int *num_doms)
+{
+	xc_domaininfo_t *dominfo;
+	xc_interface *xc_handle;
+
+	dominfo = calloc(1024, sizeof(xc_domaininfo_t));
+	if (dominfo == NULL)
+		return NULL;
+	xc_handle = xc_interface_open(0,0,0);
+	*num_doms = xc_domain_getinfolist(xc_handle, 0, 1024, dominfo);
+	xc_interface_close(xc_handle);
+	return dominfo;
+}
+
+/* Gather the qdisk statistics by querying QMP */
+void read_attributes_qdisk(xenstat_node * node)
+{
+	char *cmd_mode = "{ \"execute\": \"qmp_capabilities\" }";
+	char *query_blockstats_cmd = "{ \"execute\": \"query-blockstats\" }";
+	xc_domaininfo_t *dominfo = NULL;
+	unsigned char *qmp_stats, *val;
+	char path[80];
+	int i, qfd, num_doms;
+
+	dominfo = get_domain_ids(&num_doms);
+	if (dominfo == NULL)
+		return;
+
+	for (i=0; i<num_doms; i++) {
+		if (dominfo[i].domain <= 0)
+			continue;
+
+		/* Verify that qdisk disks are used with this VM */
+		snprintf(path, sizeof(path),"/local/domain/0/backend/qdisk/%i", dominfo[i].domain);
+		if ((val = xs_read(node->handle->xshandle, XBT_NULL, path, NULL)) == NULL)
+			continue;
+		free(val);
+
+		/* Connect to this VMs QMP socket */
+		snprintf(path, sizeof(path), "/var/run/xen/qmp-libxl-%i", dominfo[i].domain);
+		if ((qfd = qmp_connect(path)) < 0) {
+			continue;
+		}
+
+		/* First enable QMP capabilities so that we can query for data */
+		if ((qmp_stats = qmp_query(qfd, cmd_mode)) != NULL) {
+			free(qmp_stats);
+			/* Query QMP for this VMs blockstats */
+			if ((qmp_stats = qmp_query(qfd, query_blockstats_cmd)) != NULL) {
+				qmp_parse_stats(node, dominfo[i].domain, qmp_stats, qfd);
+				free(qmp_stats);
+			}
+		}
+		close(qfd);
+	}
+
+	free(dominfo);
+}
+

Re: [PATCH v2] xentop: add support for qdisks

[Ian Campbell]

On Wed, 2015-03-11 at 11:51 -0600, Charles Arnold wrote:
> Now that Xen uses qdisks by default and qemu does not write out
> statistics to sysfs this patch queries the QMP for disk statistics.
> 
> This patch depends on libyajl for parsing statistics returned from
> QMP. The runtime requires libyajl 2.0.3 or newer for required bug
> fixes in yajl_tree_parse().

Elsewhere we currently support libyajl1 even, which means that this is
all configure tests for.

You say bug fixes here, but the code comment says:
 /* Use libyajl version 2.1.x or newer for the tree parser feature with bug fixes */

which suggests it perhaps didn't even exist in earlier versions. Also I
note the quoted versions differ, FWIW.

Whether the interface exists (even in buggy form) or not in older
versions is important because if it doesn't exist then that would be a
build failure, which we would want to avoid.

Whereas a functional failure would perhaps be tolerable. However, given
the existing HAVE_YAJL_YAJL_VERSION_H define I think the code could
easily check if the YAJL library is good enough at compile time and stub
itself out -- i.e. not report qdisk stats if the yajl doesn't do the
job.

My second concern here is with the use of /var/run/xen/qmp-libxl-%i from
outside of libxl. I can't remember if qemu is safe against multiple
users of the socket. ISTR asking Anthony this before, but I don't recall
the answer, sorry :-(

Even if it is strictly speaking ok it seems a bit warty to do it, but
perhaps for an in-tree user like libxenstat it is tolerable.
Alternatively we could (relatively) easily arrange for their to be a
second qemp-libxenstat-%i socket, assuming the qemu overhead of a second
one is sane.

Would it be possible to include somewhere, either in a code comment or
in the changelog, an example of the JSON response to the QMP commands.

(I'm also consistently surprised by the lack of a qmp client library,
but that's not your fault!)

> diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c
> index 8072a90..f3847be 100644
> --- a/tools/xenstat/libxenstat/src/xenstat.c
> +++ b/tools/xenstat/libxenstat/src/xenstat.c
> @@ -657,6 +657,24 @@ static void xenstat_uninit_xen_version(xenstat_handle * handle)
>   * VBD functions
>   */
>  
> +/* Save VBD information */
> +xenstat_vbd *xenstat_save_vbd(xenstat_domain *domain, xenstat_vbd *vbd)
> +{
> +        if (domain->vbds == NULL) {
> +                domain->num_vbds = 1;
> +                domain->vbds = malloc(sizeof(xenstat_vbd));
> +        } else {
> +                domain->num_vbds++;
> +                domain->vbds = realloc(domain->vbds,
> +                                       domain->num_vbds *
> +                                       sizeof(xenstat_vbd));
> +        }

FYI realloc handles the old pointer being NULL just fine, so you don't
need to special case that so long as num_vbds starts out initialised to
0.

Also, if realloc returns NULL then you need to have remembered the old
value to free it, else it gets leaked.

> @@ -477,18 +480,10 @@ int xenstat_collect_vbds(xenstat_node * node)
>  			continue;
>  		}
>  
> -		if (domain->vbds == NULL) {
> -			domain->num_vbds = 1;
> -			domain->vbds = malloc(sizeof(xenstat_vbd));
> -		} else {
> -			domain->num_vbds++;
> -			domain->vbds = realloc(domain->vbds,
> -					       domain->num_vbds *
> -					       sizeof(xenstat_vbd));
> -		}

Oh, I see my comments above were actually on the old code you were
moving.


> +	/* Use libyajl version 2.1.x or newer for the tree parser feature with bug fixes */
> +	if ((info = yajl_tree_parse((char *)qmp_stats, errbuf, sizeof(errbuf))) == NULL) {

You don't want to log something using errbuf? If not then it may as well
be as small as possible.

> +	/* Use libyajl version 2.0.3 or newer for the tree parser feature */
> +	if ((info = yajl_tree_parse((char *)stats_buf, errbuf, sizeof(errbuf))) == NULL)

Likewise.

> +/* Get all the active domains */

actually only up to 1024 of them ;-)

Ian.

Re: [PATCH v2] xentop: add support for qdisks

[Charles Arnold]

>>> On 3/18/2015 at 10:12 AM, Ian Campbell <ian.campbell@citrix.com> wrote: 
> On Wed, 2015-03-11 at 11:51 -0600, Charles Arnold wrote:
>> Now that Xen uses qdisks by default and qemu does not write out
>> statistics to sysfs this patch queries the QMP for disk statistics.
>> 
>> This patch depends on libyajl for parsing statistics returned from
>> QMP. The runtime requires libyajl 2.0.3 or newer for required bug
>> fixes in yajl_tree_parse().
> 
> Elsewhere we currently support libyajl1 even, which means that this is
> all configure tests for.
> 
> You say bug fixes here, but the code comment says:
>  /* Use libyajl version 2.1.x or newer for the tree parser feature with bug 
> fixes */
> 
> which suggests it perhaps didn't even exist in earlier versions. Also I
> note the quoted versions differ, FWIW.

I looked it up in libyajl's ChangeLog and noticed it was fixed specifically
in 2.0.3 and noted that in the patch header but failed to go back and fix
the code comment. I'll fix the code comment.

> 
> Whether the interface exists (even in buggy form) or not in older
> versions is important because if it doesn't exist then that would be a
> build failure, which we would want to avoid.

Right. The tree feature was added in version 2.0.0 (again according
to the ChangeLog file).  I guess you would prefer not making this a
requirement in tools/configure given the statement below.

> 
> Whereas a functional failure would perhaps be tolerable. However, given
> the existing HAVE_YAJL_YAJL_VERSION_H define I think the code could
> easily check if the YAJL library is good enough at compile time and stub
> itself out -- i.e. not report qdisk stats if the yajl doesn't do the
> job.

Ok, I'll do it this way.

> 
> My second concern here is with the use of /var/run/xen/qmp-libxl-%i from
> outside of libxl. I can't remember if qemu is safe against multiple
> users of the socket. ISTR asking Anthony this before, but I don't recall
> the answer, sorry :-(
> 
> Even if it is strictly speaking ok it seems a bit warty to do it, but
> perhaps for an in-tree user like libxenstat it is tolerable.
> Alternatively we could (relatively) easily arrange for their to be a
> second qemp-libxenstat-%i socket, assuming the qemu overhead of a second
> one is sane.

As a test I modified libxl to create a qmp-libxenstat-%i socket and updated
libxenstat to use it instead of qmp-libxl-%i.  It works fine although I don't
know if there is any performance penalty for having a second socket. I am
ok with going with this solution if this is preferred.

> 
> Would it be possible to include somewhere, either in a code comment or
> in the changelog, an example of the JSON response to the QMP commands.

No problem.

> 
> (I'm also consistently surprised by the lack of a qmp client library,
> but that's not your fault!)
> 
>> diff --git a/tools/xenstat/libxenstat/src/xenstat.c 
> b/tools/xenstat/libxenstat/src/xenstat.c
>> index 8072a90..f3847be 100644
>> --- a/tools/xenstat/libxenstat/src/xenstat.c
>> +++ b/tools/xenstat/libxenstat/src/xenstat.c
>> @@ -657,6 +657,24 @@ static void xenstat_uninit_xen_version(xenstat_handle * 
> handle)
>>   * VBD functions
>>   */
>>  
>> +/* Save VBD information */
>> +xenstat_vbd *xenstat_save_vbd(xenstat_domain *domain, xenstat_vbd *vbd)
>> +{
>> +        if (domain->vbds == NULL) {
>> +                domain->num_vbds = 1;
>> +                domain->vbds = malloc(sizeof(xenstat_vbd));
>> +        } else {
>> +                domain->num_vbds++;
>> +                domain->vbds = realloc(domain->vbds,
>> +                                       domain->num_vbds *
>> +                                       sizeof(xenstat_vbd));
>> +        }
> 
> FYI realloc handles the old pointer being NULL just fine, so you don't
> need to special case that so long as num_vbds starts out initialised to
> 0.
> 
> Also, if realloc returns NULL then you need to have remembered the old
> value to free it, else it gets leaked.
> 
>> @@ -477,18 +480,10 @@ int xenstat_collect_vbds(xenstat_node * node)
>>  			continue;
>>  		}
>>  
>> -		if (domain->vbds == NULL) {
>> -			domain->num_vbds = 1;
>> -			domain->vbds = malloc(sizeof(xenstat_vbd));
>> -		} else {
>> -			domain->num_vbds++;
>> -			domain->vbds = realloc(domain->vbds,
>> -					       domain->num_vbds *
>> -					       sizeof(xenstat_vbd));
>> -		}
> 
> Oh, I see my comments above were actually on the old code you were
> moving.

I'll look at fixing this up based on your realloc comments above.

> 
> 
>> +	/* Use libyajl version 2.1.x or newer for the tree parser feature with bug 
> fixes */
>> +	if ((info = yajl_tree_parse((char *)qmp_stats, errbuf, sizeof(errbuf))) == 
> NULL) {
> 
> You don't want to log something using errbuf? If not then it may as well
> be as small as possible.

Ok.

> 
>> +	/* Use libyajl version 2.0.3 or newer for the tree parser feature */
>> +	if ((info = yajl_tree_parse((char *)stats_buf, errbuf, sizeof(errbuf))) == 
> NULL)
> 
> Likewise.

Ok.

> 
>> +/* Get all the active domains */
> 
> actually only up to 1024 of them ;-)

Ok.

Thanks for the review!

- Charles

Re: [PATCH v2] xentop: add support for qdisks

[Anthony PERARD]

On Wed, Mar 18, 2015 at 04:12:26PM +0000, Ian Campbell wrote:
> My second concern here is with the use of /var/run/xen/qmp-libxl-%i from
> outside of libxl. I can't remember if qemu is safe against multiple
> users of the socket. ISTR asking Anthony this before, but I don't recall
> the answer, sorry :-(

Last time I checked, only one client at a time can connect to the socket.
If a second user want to connect to the socket, it will be blocked until
the first one disconnect.

-- 
Anthony PERARD

Re: [PATCH v2] xentop: add support for qdisks

[Charles Arnold]

>>> On 3/19/2015 at 12:09 PM, Anthony PERARD <anthony.perard@citrix.com> wrote: 
> On Wed, Mar 18, 2015 at 04:12:26PM +0000, Ian Campbell wrote:
>> My second concern here is with the use of /var/run/xen/qmp-libxl-%i from
>> outside of libxl. I can't remember if qemu is safe against multiple
>> users of the socket. ISTR asking Anthony this before, but I don't recall
>> the answer, sorry :-(
> 
> Last time I checked, only one client at a time can connect to the socket.
> If a second user want to connect to the socket, it will be blocked until
> the first one disconnect.

This seems correct based on some of my testing.  In rare cases (perhaps not so
rare for VMs under heavy I/O load), reading the socket to get the stats times out
and so xentop will report '0' for the read/write stats until the next read attempt
that succeeds.

It looks as if we do need a second socket to qemu.  I will include that in the next
patch version.

- Charles

Re: [PATCH v2] xentop: add support for qdisks

[Ian Campbell]

On Thu, 2015-03-19 at 11:50 -0600, Charles Arnold wrote:
> > Whether the interface exists (even in buggy form) or not in older
> > versions is important because if it doesn't exist then that would be a
> > build failure, which we would want to avoid.
> 
> Right. The tree feature was added in version 2.0.0 (again according
> to the ChangeLog file).  I guess you would prefer not making this a
> requirement in tools/configure given the statement below.

Right, I think we don't want to make a global requirement for yajl >=
2.0.0 (or 2.0.3) just yet so making xenstat fallback gracefully is
probably the best option.

> > Whereas a functional failure would perhaps be tolerable. However, given
> > the existing HAVE_YAJL_YAJL_VERSION_H define I think the code could
> > easily check if the YAJL library is good enough at compile time and stub
> > itself out -- i.e. not report qdisk stats if the yajl doesn't do the
> > job.
> 
> Ok, I'll do it this way.

Thanks. FWIW if HAVE_YAJL_YAJL_VERSION_H is not set then you can assume
v1 (as libxl_json.h does).

> > My second concern here is with the use of /var/run/xen/qmp-libxl-%i from
> > outside of libxl. I can't remember if qemu is safe against multiple
> > users of the socket. ISTR asking Anthony this before, but I don't recall
> > the answer, sorry :-(
> > 
> > Even if it is strictly speaking ok it seems a bit warty to do it, but
> > perhaps for an in-tree user like libxenstat it is tolerable.
> > Alternatively we could (relatively) easily arrange for their to be a
> > second qemp-libxenstat-%i socket, assuming the qemu overhead of a second
> > one is sane.
> 
> As a test I modified libxl to create a qmp-libxenstat-%i socket and updated
> libxenstat to use it instead of qmp-libxl-%i.  It works fine although I don't
> know if there is any performance penalty for having a second socket. I am
> ok with going with this solution if this is preferred.

I'm fine with this unless someone gives a good reason not to do it.

> > Oh, I see my comments above were actually on the old code you were
> > moving.
> 
> I'll look at fixing this up based on your realloc comments above.

Thanks!

Ian.

Re: [PATCH v2] xentop: add support for qdisks

[Ian Campbell]

On Thu, 2015-03-19 at 13:20 -0600, Charles Arnold wrote:
> >>> On 3/19/2015 at 12:09 PM, Anthony PERARD <anthony.perard@citrix.com> wrote: 
> > On Wed, Mar 18, 2015 at 04:12:26PM +0000, Ian Campbell wrote:
> >> My second concern here is with the use of /var/run/xen/qmp-libxl-%i from
> >> outside of libxl. I can't remember if qemu is safe against multiple
> >> users of the socket. ISTR asking Anthony this before, but I don't recall
> >> the answer, sorry :-(
> > 
> > Last time I checked, only one client at a time can connect to the socket.
> > If a second user want to connect to the socket, it will be blocked until
> > the first one disconnect.
> 
> This seems correct based on some of my testing.  In rare cases (perhaps not so
> rare for VMs under heavy I/O load), reading the socket to get the stats times out
> and so xentop will report '0' for the read/write stats until the next read attempt
> that succeeds.
> 
> It looks as if we do need a second socket to qemu.  I will include that in the next
> patch version.

Yep, it sounds like it is a requirement, thanks.

Ian.