Skip to contentSkip to conversation

aio event fd support to blktap2

PATCH In review Daniel Stodden 5 participants 32 messages Jan 28, 2010 – Feb 3, 2010 2 messages not in the archive

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 aio event fd support to blktap2

Get blktap2 running on pvops.

This mainly adds eventfd support to the userland code. Based on some prior cleanup to tapdisk-queue and the server object. We had most of that in XenServer for a while, so I kept it stacked.

1. Clean up IPC and AIO init in tapdisk-server.

[I think tapdisk-ipc in blktap2 is basically obsolete.
Pending a later patch to remove it?]

2. Split tapdisk-queue into variable raw I/O backends.

This basically makes an 'ops'-struct (struct tio) out of what used to be primarily libaio vs. an if/else hack to resort to canonical read()/write()s where desirable.

For now, the one chosen remains as hardcoded as ever.

3. Prefer AIO eventfd support on kernels >= 2.6.22

Mainline Linux after 2.6.22 finally got I/O muxing for AIO. Unfortunately, few systems bring the necessary libaio update (0.3.107), xen/tools included.

Since this is just about a bunch of inline macros and an update to reserved space in the iocb struct, let's add a compat header with private typedefs instead.

This should obsolete tools/aio.

Misc:
- Does a runtime kernel version check. I guess this code will need additional cpp magic on BSDs.
- Wants a PERROR macro in blktaplib.h
- Fixes a bug in tapdisk-vbd which locks up the sync io mode.
- Removed dead code in qcow2raw to make it link again.

#1 lore raw

PATCH 1/1 blktap2: Sort out tapdisk IPC init

# HG changeset patch
# User Daniel Stodden <[email protected]> # Date 1258172773 28800
# Node ID 9da64803841f4d8d8816f6d0218cbc6b672f7903 # Parent 2636e561970898517def148c19e04581b12dc860
blktap2: Sort out tapdisk IPC init.

Move I/O and event callbacks setup out of tapdisk-server, into tapdisk-ipc.

tools/blktap2/drivers/tapdisk-ipc.c +74 -0
diff -r 2636e5619708 -r 9da64803841f tools/blktap2/drivers/tapdisk-ipc.c
@@ -30,12 +30,86 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <string.h>
+#include <fcntl.h>
 
 #include "tapdisk.h"
 #include "tapdisk-ipc.h"
 #include "tapdisk-vbd.h"
 #include "tapdisk-server.h"
 
+static void
+tapdisk_ipc_read_event(event_id_t id, char mode, void *private)
+{
+	td_ipc_t *ipc = private;
+	tapdisk_ipc_read(ipc);
+}
+
+static void
+__tapdisk_ipc_init(td_ipc_t *ipc)
+{
+	ipc->rfd = -1;
+	ipc->wfd = -1;
+	ipc->rfd_event = -1;
+}
+
+int
+tapdisk_ipc_open(td_ipc_t *ipc, const char *read, const char *write)
+{
+	int err;
+
+	memset(ipc, 0, sizeof(td_ipc_t));
+	__tapdisk_ipc_init(ipc);
+
+	if (read) {
+		ipc->rfd = open(read, O_RDWR | O_NONBLOCK);
+		if (ipc->rfd < 0) {
+			err = -errno;
+			EPRINTF("FD open failed %s: %d\n", read, err);
+			goto fail;
+		}
+
+		ipc->rfd_event = 
+			tapdisk_server_register_event(SCHEDULER_POLL_READ_FD,
+						      ipc->rfd, 0,
+						      tapdisk_ipc_read_event,
+						      ipc);
+		if (ipc->rfd_event < 0) {
+			err = ipc->rfd_event;
+			goto fail;
+		}
+	}
+
+	if (write) {
+		ipc->wfd = open(write, O_RDWR | O_NONBLOCK);
+		if (ipc->wfd < 0) {
+			err = -errno;
+			EPRINTF("FD open failed %s, %d\n", write, err);
+			goto fail;
+		}
+	}
+
+	return 0;
+
+fail:
+	tapdisk_ipc_close(ipc);
+	return err;
+}
+
+void
+tapdisk_ipc_close(td_ipc_t *ipc)
+{
+	if (ipc->rfd > 0)
+		close(ipc->rfd);
+
+	if (ipc->wfd > 0)
+		close(ipc->wfd);
+
+	if (ipc->rfd_event >= 0)
+		tapdisk_server_unregister_event(ipc->rfd_event);
+
+	__tapdisk_ipc_init(ipc);
+}
+
 static int
 tapdisk_ipc_write_message(int fd, tapdisk_message_t *message, int timeout)
 {
tools/blktap2/drivers/tapdisk-ipc.h +4 -0
diff -r 2636e5619708 -r 9da64803841f tools/blktap2/drivers/tapdisk-ipc.h
@@ -29,13 +29,17 @@
 #define _TAPDISK_IPC_H_
 
 #include "tapdisk-message.h"
+#include "scheduler.h"
 
 typedef struct td_ipc_handle {
 	int                         rfd;
 	int                         wfd;
+	event_id_t		    rfd_event;
 	td_uuid_t                   uuid;
 } td_ipc_t;
 
+int tapdisk_ipc_open(td_ipc_t *ipc, const char *read, const char *write);
+void tapdisk_ipc_close(td_ipc_t *ipc);
 int tapdisk_ipc_read(td_ipc_t *ipc);
 int tapdisk_ipc_write(td_ipc_t *ipc, int type);
 int tapdisk_ipc_write_error(td_ipc_t *ipc, const char *message);
tools/blktap2/drivers/tapdisk-server.c +17 -57

120 lines not on this page · raw message

tools/blktap2/drivers/tapdisk-server.h +0 -1
diff -r 2636e5619708 -r 9da64803841f tools/blktap2/drivers/tapdisk-server.h
@@ -57,7 +57,6 @@
 	td_ipc_t                     ipc;
 	struct list_head             vbds;
 	scheduler_t                  scheduler;
-	event_id_t                   control_event;
 	struct tqueue                aio_queue;
 	event_id_t                   aio_queue_event_id;
 } tapdisk_server_t;
#2 lore raw

PATCH 1/1 blktap2: Sort out tapdisk AIO init

In reply to Daniel Stodden

# HG changeset patch
# User Daniel Stodden <[email protected]> # Date 1258172774 28800
# Node ID 2439e941318745b333e63493af247b9558ddebc2 # Parent 9da64803841f4d8d8816f6d0218cbc6b672f7903
blktap2: Sort out tapdisk AIO init.

Move event callbacks registration into tapdisk-queue. This should also obsoletes the dummy pollfd pipe in the synchronous I/O case.

tools/blktap2/drivers/io-optimize.c +7 -0
diff -r 9da64803841f -r 2439e9413187 tools/blktap2/drivers/io-optimize.c
@@ -51,9 +51,16 @@
 opio_free(struct opioctx *ctx)
 {
 	free(ctx->opios);
+	ctx->opios = NULL;
+
 	free(ctx->free_opios);
+	ctx->free_opios = NULL;
+
 	free(ctx->iocb_queue);
+	ctx->iocb_queue = NULL;
+
 	free(ctx->event_queue);
+	ctx->event_queue = NULL;
 }
 
 int
tools/blktap2/drivers/qcow2raw.c +0 -6
diff -r 9da64803841f -r 2439e9413187 tools/blktap2/drivers/qcow2raw.c
@@ -101,12 +101,6 @@
     return;
 }
 
-void
-queue_event(event_id_t id, char mode, void *private)
-{
-  tapdisk_complete_tiocbs(&server.aio_queue);
-}
-
 static void debug_output(uint64_t progress, uint64_t size)
 {
         //Output progress every PROGRESS_QUANT 
tools/blktap2/drivers/tapdisk-queue.c +46 -19

120 lines not on this page · raw message

tools/blktap2/drivers/tapdisk-queue.h +2 -2
diff -r 9da64803841f -r 2439e9413187 tools/blktap2/drivers/tapdisk-queue.h
@@ -32,6 +32,7 @@
 #include <libaio.h>
 
 #include "io-optimize.h"
+#include "scheduler.h"
 
 struct tiocb;
 struct tfilter;
@@ -57,9 +58,9 @@
 	int                   sync;
 
 	int                   poll_fd;
+	event_id_t	      event;
 	io_context_t          aio_ctx;
 	struct opioctx        opioctx;
-	int                   dummy_pipe[2];
 
 	int                   queued;
 	struct iocb         **iocbs;
@@ -104,7 +105,6 @@
 void tapdisk_queue_tiocb(struct tqueue *, struct tiocb *);
 int tapdisk_submit_tiocbs(struct tqueue *);
 int tapdisk_submit_all_tiocbs(struct tqueue *);
-int tapdisk_complete_tiocbs(struct tqueue *);
 int tapdisk_cancel_tiocbs(struct tqueue *);
 int tapdisk_cancel_all_tiocbs(struct tqueue *);
 void tapdisk_prep_tiocb(struct tiocb *, int, int, char *, size_t,
tools/blktap2/drivers/tapdisk-server.c +9 -35

69 lines not on this page · raw message

tools/blktap2/drivers/tapdisk-server.h +0 -1
diff -r 9da64803841f -r 2439e9413187 tools/blktap2/drivers/tapdisk-server.h
@@ -58,7 +58,6 @@
 	struct list_head             vbds;
 	scheduler_t                  scheduler;
 	struct tqueue                aio_queue;
-	event_id_t                   aio_queue_event_id;
 } tapdisk_server_t;
 
 #endif
#3 lore raw

PATCH 1/1 blktap2: Separate tapdisk raw I/O into different backends

In reply to Daniel Stodden

# HG changeset patch
# User Daniel Stodden <[email protected]> # Date 1264717794 28800
# Node ID b816b4a752d8d2dcc6e7e01ad0cf75404932d94b # Parent 2439e941318745b333e63493af247b9558ddebc2
blktap2: Separate tapdisk raw I/O into different backends.

Hide tapdisk support for different raw I/O interfaces behind a new struct tio. Libaio remains to dominate the interface, requiring everyone to dispatch iocb/ioevent structs.

Backends:
- lio: Kernel AIO via libaio.
- rwio: Canonical read/write() mode.

Misc:
- Fixes a bug in tapdisk-vbd which locks up the sync io mode.
- Wants a PERROR macro in blktaplib.h
- Removes dead code in qcow2raw to make it link again.

tools/blktap2/drivers/tapdisk-queue.c +283 -130
diff -r 2439e9413187 -r b816b4a752d8 tools/blktap2/drivers/tapdisk-queue.c
@@ -141,7 +141,7 @@
 	 * use a private linked list to keep track
 	 * of the tiocbs we're cancelling. 
 	 */
-	tiocb  = (struct tiocb *)queue->iocbs[0]->data;
+	tiocb  = queue->iocbs[0]->data;
 	queued = queue->queued;
 	queue->queued = 0;
 
@@ -165,8 +165,40 @@
 	return cancel_tiocbs(queue, err);
 }
 
+/*
+ * rwio
+ */
+
+struct rwio {
+	struct io_event *aio_events;
+};
+
+static void
+tapdisk_rwio_destroy(struct tqueue *queue)
+{
+	struct rwio *rwio = queue->tio_data;
+
+	if (rwio->aio_events) {
+		free(rwio->aio_events);
+		rwio->aio_events = NULL;
+	}
+}
+
+static int
+tapdisk_rwio_setup(struct tqueue *queue, int size)
+{
+	struct rwio *rwio = queue->tio_data;
+	int err;
+
+	rwio->aio_events = calloc(size, sizeof(struct io_event));
+	if (!rwio->aio_events)
+		return -errno;
+
+	return 0;
+}
+
 static inline ssize_t
-iocb_rw(struct iocb *iocb)
+tapdisk_rwio_rw(const struct iocb *iocb)
 {
 	int fd        = iocb->aio_fildes;
 	char *buf     = iocb->u.c.buf;
@@ -177,7 +209,7 @@
 
 	if (lseek(fd, off, SEEK_SET) == (off_t)-1)
 		return -errno;
-	
+
 	if (atomicio(func, fd, buf, size) != size)
 		return -errno;
 
@@ -185,8 +217,9 @@
 }
 
 static int
-io_synchronous_rw(struct tqueue *queue)
+tapdisk_rwio_submit(struct tqueue *queue)
 {
+	struct rwio *rwio = queue->tio_data;
 	int i, merged, split;
 	struct iocb *iocb;
 	struct tiocb *tiocb;
@@ -201,18 +234,18 @@
 	queue->queued = 0;
 
 	for (i = 0; i < merged; i++) {
-		ep      = queue->aio_events + i;
+		ep      = rwio->aio_events + i;
 		iocb    = queue->iocbs[i];
 		ep->obj = iocb;
-		ep->res = iocb_rw(iocb);
+		ep->res = tapdisk_rwio_rw(iocb);
 	}
 
-	split = io_split(&queue->opioctx, queue->aio_events, merged);
-	tapdisk_filter_events(queue->filter, queue->aio_events, split);
+	split = io_split(&queue->opioctx, rwio->aio_events, merged);
+	tapdisk_filter_events(queue->filter, rwio->aio_events, split);
 
-	for (i = split, ep = queue->aio_events; i-- > 0; ep++) {
+	for (i = split, ep = rwio->aio_events; i-- > 0; ep++) {
 		iocb  = ep->obj;
-		tiocb = (struct tiocb *)iocb->data;
+		tiocb = iocb->data;
 		complete_tiocb(queue, tiocb, ep->res);
 	}
 
@@ -221,65 +254,258 @@
 	return split;
 }
 
-static void tapdisk_tiocb_event(event_id_t id, char mode, void *private);
+static const struct tio td_tio_rwio = {
+	.name        = "rwio",
+	.data_size   = 0,
+	.tio_setup   = NULL,
+	.tio_destroy = NULL,
+	.tio_submit  = tapdisk_rwio_submit
+};
+
+/*
+ * libaio
+ */
+
+struct lio {
+	io_context_t     aio_ctx;
+	struct io_event *aio_events;
+
+	int              poll_fd;
+	int              event_id;
+};
+
+static void
+tapdisk_lio_destroy(struct tqueue *queue)
+{
+	struct lio *lio = queue->tio_data;
+
+	if (!lio)
+		return;
+
+	if (lio->event_id >= 0) {
+		tapdisk_server_unregister_event(lio->event_id);
+		lio->event_id = -1;
+	}
+
+	if (lio->aio_ctx) {
+		io_destroy(lio->aio_ctx);
+		lio->aio_ctx = NULL;
+	}
+
+	if (lio->aio_events) {
+		free(lio->aio_events);
+		lio->aio_events = NULL;
+	}
+}
+
+static void
+tapdisk_lio_event(event_id_t id, char mode, void *private)
+{
+	struct tqueue *queue = private;
+	struct lio *lio;
+	int i, ret, split;
+	struct iocb *iocb;
+	struct tiocb *tiocb;
+	struct io_event *ep;
+
+	lio   = queue->tio_data;
+	ret   = io_getevents(lio->aio_ctx, 0,
+			     queue->size, lio->aio_events, NULL);
+	split = io_split(&queue->opioctx, lio->aio_events, ret);
+	tapdisk_filter_events(queue->filter, lio->aio_events, split);
+
+	DBG("events: %d, tiocbs: %d\n", ret, split);
+
+	queue->iocbs_pending  -= ret;
+	queue->tiocbs_pending -= split;
+
+	for (i = split, ep = lio->aio_events; i-- > 0; ep++) {
+		iocb  = ep->obj;
+		tiocb = iocb->data;
+		complete_tiocb(queue, tiocb, ep->res);
+	}
+
+	queue_deferred_tiocbs(queue);
+}
+
+static int
+tapdisk_lio_setup(struct tqueue *queue, int qlen)
+{
+	struct lio *lio = queue->tio_data;
+	size_t sz;
+	int err;
+
+	lio->event_id = -1;
+	lio->aio_ctx  = REQUEST_ASYNC_FD;
+
+	lio->poll_fd = io_setup(qlen, &lio->aio_ctx);
+	err = lio->poll_fd;
+	if (err < 0) {
+		lio->aio_ctx = NULL;
+
+		if (err == -EAGAIN)
+			goto fail_rsv;
+
+		goto fail_fd;
+	}
+
+	lio->event_id =
+		tapdisk_server_register_event(SCHEDULER_POLL_READ_FD,
+					      lio->poll_fd, 0,
+					      tapdisk_lio_event,
+					      queue);
+	err = lio->event_id;
+	if (err < 0)
+		goto fail;
+
+	lio->aio_events = calloc(qlen, sizeof(struct io_event));
+	if (!lio->aio_events) {
+		err = -errno;
+		goto fail;
+	}
+
+	return 0;
+
+fail:
+	tapdisk_lio_destroy(queue);
+	return err;
+
+fail_rsv:
+	DPRINTF("Couldn't setup AIO context. If you are trying to "
+		"concurrently use a large number of blktap-based disks, you may "
+		"need to increase the system-wide aio request limit. "
+		"(e.g. 'echo 1048576 > /proc/sys/fs/aio-max-nr')\n");
+	goto fail;
+
+fail_fd:
+	DPRINTF("Couldn't get fd for AIO poll support. This is probably "
+		"because your kernel does not have  the aio-poll patch "
+		"applied.\n");
+	goto fail;
+}
+
+static int
+tapdisk_lio_submit(struct tqueue *queue)
+{
+	struct lio *lio = queue->tio_data;
+	int merged, submitted, err = 0;
+
+	if (!queue->queued)
+		return 0;
+
+	tapdisk_filter_iocbs(queue->filter, queue->iocbs, queue->queued);
+	merged    = io_merge(&queue->opioctx, queue->iocbs, queue->queued);
+	submitted = io_submit(lio->aio_ctx, merged, queue->iocbs);
+
+	DBG("queued: %d, merged: %d, submitted: %d\n",
+	    queue->queued, merged, submitted);
+
+	if (submitted < 0) {
+		err = submitted;
+		submitted = 0;
+	} else if (submitted < merged)
+		err = -EIO;
+
+	queue->iocbs_pending  += submitted;
+	queue->tiocbs_pending += queue->queued;
+	queue->queued          = 0;
+
+	if (err)
+		queue->tiocbs_pending -= 
+			fail_tiocbs(queue, submitted, merged, err);
+
+	return submitted;
+}
+
+static const struct tio td_tio_lio = {
+	.name        = "lio",
+	.data_size   = sizeof(struct lio),
+	.tio_setup   = tapdisk_lio_setup,
+	.tio_destroy = tapdisk_lio_destroy,
+	.tio_submit  = tapdisk_lio_submit,
+};
+
+static void
+tapdisk_queue_free_io(struct tqueue *queue)
+{
+	if (queue->tio) {
+		if (queue->tio->tio_destroy)
+			queue->tio->tio_destroy(queue);
+		queue->tio = NULL;
+	}
+
+	if (queue->tio_data) {
+		free(queue->tio_data);
+		queue->tio_data = NULL;
+	}
+}
+
+static int
+tapdisk_queue_init_io(struct tqueue *queue, int drv)
+{
+	const struct tio *tio;
+	int err;
+
+	switch (drv) {
+	case TIO_DRV_LIO:
+		tio = &td_tio_lio;
+		break;
+	case TIO_DRV_RWIO:
+		tio = &td_tio_rwio;
+		break;
+	default:
+		err = -EINVAL;
+		goto fail;
+	}
+
+	queue->tio_data = calloc(1, tio->data_size);
+	if (!queue->tio_data) {
+		PERROR("malloc(%zu)", tio->data_size);
+		err = -errno;
+		goto fail;
+	}
+
+	queue->tio = tio;
+
+	if (tio->tio_setup) {
+		err = tio->tio_setup(queue, queue->size);
+		if (err)
+			goto fail;
+	}
+
+	DPRINTF("I/O queue driver: %s\n", tio->name);
+
+	return 0;
+
+fail:
+	tapdisk_queue_free_io(queue);
+	return err;
+}
 
 int
 tapdisk_init_queue(struct tqueue *queue, int size,
-		   int sync, struct tfilter *filter)
+		   int drv, struct tfilter *filter)
 {
 	int i, err;
 
 	memset(queue, 0, sizeof(struct tqueue));
 
 	queue->size   = size;
-	queue->sync   = sync;
 	queue->filter = filter;
 
-	queue->event   = -1;
-	queue->aio_ctx = NULL;
-
 	if (!size)
 		return 0;
 
-	if (!sync) {
-		queue->aio_ctx = REQUEST_ASYNC_FD;
-		queue->poll_fd = io_setup(size, &queue->aio_ctx);
-		err = queue->poll_fd;
-		if (err < 0) {
-			if (err == -EAGAIN)
-				DPRINTF("Couldn't setup AIO context.  If you "
-					"are trying to concurrently use a "
-					"large number of blktap-based disks, "
-					"you may need to increase the "
-					"system-wide aio request limit. "
-					"(e.g. 'echo 1048576 > /proc/sys/fs/"
-					"aio-max-nr')\n");
-			else
-				DPRINTF("Couldn't get fd for AIO poll "
-					"support.  This is probably because "
-					"your kernel does not have the "
-					"aio-poll patch applied.\n");
-			queue->aio_ctx = NULL;
-			goto fail;
-		}
+	err = tapdisk_queue_init_io(queue, drv);
+	if (err)
+		goto fail;
 
-		queue->event =
-			tapdisk_server_register_event(SCHEDULER_POLL_READ_FD,
-						      queue->poll_fd, 0,
-						      tapdisk_tiocb_event,
-						      queue);
-		err = queue->event;
-		if (err < 0)
-			goto fail;
-
+	queue->iocbs = calloc(size, sizeof(struct iocb *));
+	if (!queue->iocbs) {
+		err = -errno;
+		goto fail;
 	}
 
-	err               = -ENOMEM;
-	queue->iocbs      = calloc(size, sizeof(struct iocb *));
-	queue->aio_events = calloc(size, sizeof(struct io_event));
-	if (!queue->iocbs || !queue->aio_events)
-		goto fail;
-
 	err = opio_init(&queue->opioctx, size);
 	if (err)
 		goto fail;
@@ -294,22 +520,11 @@
 void
 tapdisk_free_queue(struct tqueue *queue)
 {
-	if (queue->event >= 0) {
-		tapdisk_server_unregister_event(queue->event);
-		queue->event = -1;
-	}
-
-	if (queue->aio_ctx) {
-		io_destroy(queue->aio_ctx);
-		queue->aio_ctx = NULL;
-	}
+	tapdisk_queue_free_io(queue);
 
 	free(queue->iocbs);
 	queue->iocbs = NULL;
 
-	free(queue->aio_events);
-	queue->aio_events = NULL;
-
 	opio_free(&queue->opioctx);
 }
 
@@ -319,9 +534,9 @@
 	struct tiocb *tiocb = queue->deferred.head;
 
 	WARN("TAPDISK QUEUE:\n");
-	WARN("size: %d, sync: %d, queued: %d, iocbs_pending: %d, "
+	WARN("size: %d, tio: %s, queued: %d, iocbs_pending: %d, "
 	     "tiocbs_pending: %d, tiocbs_deferred: %d, deferrals: %"PRIx64"\n",
-	     queue->size, queue->sync, queue->queued, queue->iocbs_pending,
+	     queue->size, queue->tio->name, queue->queued, queue->iocbs_pending,
 	     queue->tiocbs_pending, queue->tiocbs_deferred, queue->deferrals);
 
 	if (tiocb) {
@@ -362,42 +577,14 @@
 		defer_tiocb(queue, tiocb);
 }
 
+
 /*
  * fail_tiocbs may queue more tiocbs
  */
 int
 tapdisk_submit_tiocbs(struct tqueue *queue)
 {
-	int merged, submitted, err = 0;
-
-	if (!queue->queued)
-		return 0;
-
-	if (queue->sync)
-		return io_synchronous_rw(queue);
-
-	tapdisk_filter_iocbs(queue->filter, queue->iocbs, queue->queued);
-	merged    = io_merge(&queue->opioctx, queue->iocbs, queue->queued);
-	submitted = io_submit(queue->aio_ctx, merged, queue->iocbs);
-
-	DBG("queued: %d, merged: %d, submitted: %d\n",
-	    queue->queued, merged, submitted);
-
-	if (submitted < 0) {
-		err = submitted;
-		submitted = 0;
-	} else if (submitted < merged)
-		err = -EIO;
-
-	queue->iocbs_pending  += submitted;
-	queue->tiocbs_pending += queue->queued;
-	queue->queued          = 0;
-
-	if (err)
-		queue->tiocbs_pending -= 
-			fail_tiocbs(queue, submitted, merged, err);
-
-	return submitted;
+	return queue->tio->tio_submit(queue);
 }
 
 int
@@ -412,40 +599,6 @@
 	return submitted;
 }
 
-static void
-tapdisk_complete_tiocbs(struct tqueue *queue)
-{
-	int i, ret, split;
-	struct iocb *iocb;
-	struct tiocb *tiocb;
-	struct io_event *ep;
-
-	ret   = io_getevents(queue->aio_ctx, 0,
-			     queue->size, queue->aio_events, NULL);
-	split = io_split(&queue->opioctx, queue->aio_events, ret);
-	tapdisk_filter_events(queue->filter, queue->aio_events, split);
-
-	DBG("events: %d, tiocbs: %d\n", ret, split);
-
-	queue->iocbs_pending  -= ret;
-	queue->tiocbs_pending -= split;
-
-	for (i = split, ep = queue->aio_events; i-- > 0; ep++) {
-		iocb  = ep->obj;
-		tiocb = (struct tiocb *)iocb->data;
-		complete_tiocb(queue, tiocb, ep->res);
-	}
-
-	queue_deferred_tiocbs(queue);
-}
-
-static void
-tapdisk_tiocb_event(event_id_t id, char mode, void *private)
-{
-	struct tqueue *queue = private;
-	tapdisk_complete_tiocbs(queue);
-}
-
 /*
  * cancel_tiocbs may queue more tiocbs
  */
tools/blktap2/drivers/tapdisk-queue.h +18 -6
diff -r 2439e9413187 -r b816b4a752d8 tools/blktap2/drivers/tapdisk-queue.h
@@ -55,16 +55,14 @@
 
 struct tqueue {
 	int                   size;
-	int                   sync;
 
-	int                   poll_fd;
-	event_id_t	      event;
-	io_context_t          aio_ctx;
+	const struct tio     *tio;
+	void                 *tio_data;
+
 	struct opioctx        opioctx;
 
 	int                   queued;
 	struct iocb         **iocbs;
-	struct io_event      *aio_events;
 
 	/* number of iocbs pending in the aio layer */
 	int                   iocbs_pending;
@@ -86,6 +84,20 @@
 	uint64_t              deferrals;
 };
 
+struct tio {
+	const char           *name;
+	size_t                data_size;
+
+	int  (*tio_setup)    (struct tqueue *queue, int qlen);
+	void (*tio_destroy)  (struct tqueue *queue);
+	int  (*tio_submit)   (struct tqueue *queue);
+};
+
+enum {
+	TIO_DRV_LIO     = 1,
+	TIO_DRV_RWIO    = 2,
+};
+
 /*
  * Interface for request producer (i.e., tapdisk)
  * NB: the following functions may cause additional tiocbs to be queued:
@@ -99,7 +111,7 @@
 #define tapdisk_queue_empty(q) ((q)->queued == 0)
 #define tapdisk_queue_full(q)  \
 	(((q)->tiocbs_pending + (q)->queued) >= (q)->size)
-int tapdisk_init_queue(struct tqueue *, int size, int sync, struct tfilter *);
+int tapdisk_init_queue(struct tqueue *, int size, int drv, struct tfilter *);
 void tapdisk_free_queue(struct tqueue *);
 void tapdisk_debug_queue(struct tqueue *);
 void tapdisk_queue_tiocb(struct tqueue *, struct tiocb *);
tools/blktap2/drivers/tapdisk-server.c +2 -1
diff -r 2439e9413187 -r b816b4a752d8 tools/blktap2/drivers/tapdisk-server.c
@@ -236,7 +236,8 @@
 static int
 tapdisk_server_init_aio(void)
 {
-	return tapdisk_init_queue(&server.aio_queue, TAPDISK_TIOCBS, 0, NULL);
+	return tapdisk_init_queue(&server.aio_queue, TAPDISK_TIOCBS,
+				  TIO_DRV_LIO, NULL);
 }
 
 static void
tools/blktap2/drivers/tapdisk-vbd.c +2 -0
diff -r 2439e9413187 -r b816b4a752d8 tools/blktap2/drivers/tapdisk-vbd.c
@@ -1260,6 +1260,8 @@
 	int n;
 	td_ring_t *ring;
 
+	tapdisk_vbd_check_state(vbd);
+
 	ring = &vbd->ring;
 	if (!ring->sring)
 		return 0;
tools/blktap2/include/blktaplib.h +1 -0
diff -r 2439e9413187 -r b816b4a752d8 tools/blktap2/include/blktaplib.h
@@ -43,6 +43,7 @@
 #endif
 
 #define EPRINTF(_f, _a...) syslog(LOG_ERR, "tap-err:%s: " _f, __func__, ##_a)
+#define PERROR(_f, _a...)  EPRINTF(_f ": %s", ##_a, strerror(errno))
 
 #define BLK_RING_SIZE __RING_SIZE((blkif_sring_t *)0, XC_PAGE_SIZE)
#4 lore raw

PATCH 1/1 blktap2: Prefer AIO eventfd support on kernels >= 2.6.22

In reply to Daniel Stodden

# HG changeset patch
# User Daniel Stodden <[email protected]> # Date 1264717884 28800
# Node ID b7d6bbba856b7a3c5170f7377a3d490429a91ba5 # Parent b816b4a752d8d2dcc6e7e01ad0cf75404932d94b
blktap2: Prefer AIO eventfd support on kernels >= 2.6.22

Mainline kernel support for eventfd(2) in linux aio was added between 2.6.21 and 2.6.22. Libaio after 0.3.107 has the header file, but presently few systems support it. Neither do we rely on an up-to-date libc6.

Instead, this patch adds a header which defines custom iocb_common struct, and works around a potentially missing sys/eventfd.h. q

tools/blktap2/drivers/block-aio.c +0 -1
diff -r b816b4a752d8 -r b7d6bbba856b tools/blktap2/drivers/block-aio.c
@@ -28,7 +28,6 @@
 
 
 #include <errno.h>
-#include <libaio.h>
 #include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
tools/blktap2/drivers/libaio-compat.h new file +92 -0

92 lines not on this page · raw message

tools/blktap2/drivers/tapdisk-queue.c +148 -30

239 lines not on this page · raw message

tools/blktap2/drivers/tapdisk-utils.c +32 -0
diff -r b816b4a752d8 -r b7d6bbba856b tools/blktap2/drivers/tapdisk-utils.c
@@ -33,6 +33,10 @@
 #include <sys/mman.h>
 #include <sys/ioctl.h>
 #include <sys/resource.h>
+#include <sys/utsname.h>
+#ifdef __linux__
+#include <linux/version.h>
+#endif
 
 #include "blk.h"
 #include "tapdisk.h"
@@ -183,3 +187,31 @@
 
 	return 0;
 }
+
+#ifdef __linux__
+
+int tapdisk_linux_version(void)
+{
+	struct utsname uts;
+	unsigned int version, patchlevel, sublevel;
+	int n, err;
+
+	err = uname(&uts);
+	if (err)
+		return -errno;
+
+	n = sscanf(uts.release, "%u.%u.%u", &version, &patchlevel, &sublevel);
+	if (n != 3)
+		return -ENOSYS;
+
+	return KERNEL_VERSION(version, patchlevel, sublevel);
+}
+
+#else
+
+int tapdisk_linux_version(void)
+{
+	return -ENOSYS;
+}
+
+#endif
tools/blktap2/drivers/tapdisk-utils.h +1 -0
diff -r b816b4a752d8 -r b7d6bbba856b tools/blktap2/drivers/tapdisk-utils.h
@@ -38,5 +38,6 @@
 int tapdisk_namedup(char **, const char *);
 int tapdisk_parse_disk_type(const char *, char **, int *);
 int tapdisk_get_image_size(int, uint64_t *, uint32_t *);
+int tapdisk_linux_version(void);
 
 #endif
#5 lore raw
In reply to Daniel Stodden
Quoted 5 lines · Daniel Stodden

Get blktap2 running on pvops.

This mainly adds eventfd support to the userland code. Based on some prior cleanup to tapdisk-queue and the server object. We had most of that in XenServer for a while, so I kept it stacked.

Patches 1, 2 and 4 are missing a signed-off-by line. I could not apply patch 3 because it was rejected as mangled by 'patch': not sure why this happens from time to time, but please re-send all 4 patches as attachments, and include signed-off-by on each one.

Thanks,
Keir

#6 lore raw

PATCH 1/1 aio event fd support to blktap2

Is --inline right/better?

Cheers,
Daniel

#7 lore raw

PATCH 1/1 blktap2: Sort out tapdisk IPC init

# HG changeset patch
# User Daniel Stodden <[email protected]> # Date 1258172773 28800
# Node ID aa0d1795f1ecfdc7e235b9ada87fb7a71e455f7f # Parent aa1ad4c5c931e0c402dc2c8356d1604a156d02f0
blktap2: Sort out tapdisk IPC init.

Move I/O and event callbacks setup out of tapdisk-server, into tapdisk-ipc.

tools/blktap2/drivers/tapdisk-ipc.c +74 -0
diff -r aa1ad4c5c931 -r aa0d1795f1ec tools/blktap2/drivers/tapdisk-ipc.c
@@ -30,12 +30,86 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <string.h>
+#include <fcntl.h>
 
 #include "tapdisk.h"
 #include "tapdisk-ipc.h"
 #include "tapdisk-vbd.h"
 #include "tapdisk-server.h"
 
+static void
+tapdisk_ipc_read_event(event_id_t id, char mode, void *private)
+{
+	td_ipc_t *ipc = private;
+	tapdisk_ipc_read(ipc);
+}
+
+static void
+__tapdisk_ipc_init(td_ipc_t *ipc)
+{
+	ipc->rfd = -1;
+	ipc->wfd = -1;
+	ipc->rfd_event = -1;
+}
+
+int
+tapdisk_ipc_open(td_ipc_t *ipc, const char *read, const char *write)
+{
+	int err;
+
+	memset(ipc, 0, sizeof(td_ipc_t));
+	__tapdisk_ipc_init(ipc);
+
+	if (read) {
+		ipc->rfd = open(read, O_RDWR | O_NONBLOCK);
+		if (ipc->rfd < 0) {
+			err = -errno;
+			EPRINTF("FD open failed %s: %d\n", read, err);
+			goto fail;
+		}
+
+		ipc->rfd_event = 
+			tapdisk_server_register_event(SCHEDULER_POLL_READ_FD,
+						      ipc->rfd, 0,
+						      tapdisk_ipc_read_event,
+						      ipc);
+		if (ipc->rfd_event < 0) {
+			err = ipc->rfd_event;
+			goto fail;
+		}
+	}
+
+	if (write) {
+		ipc->wfd = open(write, O_RDWR | O_NONBLOCK);
+		if (ipc->wfd < 0) {
+			err = -errno;
+			EPRINTF("FD open failed %s, %d\n", write, err);
+			goto fail;
+		}
+	}
+
+	return 0;
+
+fail:
+	tapdisk_ipc_close(ipc);
+	return err;
+}
+
+void
+tapdisk_ipc_close(td_ipc_t *ipc)
+{
+	if (ipc->rfd > 0)
+		close(ipc->rfd);
+
+	if (ipc->wfd > 0)
+		close(ipc->wfd);
+
+	if (ipc->rfd_event >= 0)
+		tapdisk_server_unregister_event(ipc->rfd_event);
+
+	__tapdisk_ipc_init(ipc);
+}
+
 static int
 tapdisk_ipc_write_message(int fd, tapdisk_message_t *message, int timeout)
 {
tools/blktap2/drivers/tapdisk-ipc.h +4 -0
diff -r aa1ad4c5c931 -r aa0d1795f1ec tools/blktap2/drivers/tapdisk-ipc.h
@@ -29,13 +29,17 @@
 #define _TAPDISK_IPC_H_
 
 #include "tapdisk-message.h"
+#include "scheduler.h"
 
 typedef struct td_ipc_handle {
 	int                         rfd;
 	int                         wfd;
+	event_id_t		    rfd_event;
 	td_uuid_t                   uuid;
 } td_ipc_t;
 
+int tapdisk_ipc_open(td_ipc_t *ipc, const char *read, const char *write);
+void tapdisk_ipc_close(td_ipc_t *ipc);
 int tapdisk_ipc_read(td_ipc_t *ipc);
 int tapdisk_ipc_write(td_ipc_t *ipc, int type);
 int tapdisk_ipc_write_error(td_ipc_t *ipc, const char *message);
tools/blktap2/drivers/tapdisk-server.c +17 -57

120 lines not on this page · raw message

tools/blktap2/drivers/tapdisk-server.h +0 -1
diff -r aa1ad4c5c931 -r aa0d1795f1ec tools/blktap2/drivers/tapdisk-server.h
@@ -57,7 +57,6 @@
 	td_ipc_t                     ipc;
 	struct list_head             vbds;
 	scheduler_t                  scheduler;
-	event_id_t                   control_event;
 	struct tqueue                aio_queue;
 	event_id_t                   aio_queue_event_id;
 } tapdisk_server_t;

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

#8 lore raw

PATCH 1/1 blktap2: Sort out tapdisk AIO init

In reply to Daniel Stodden

# HG changeset patch
# User Daniel Stodden <[email protected]> # Date 1258172774 28800
# Node ID 9746b010e2bf495a1c93c37ab98f9d1fdde559df # Parent aa0d1795f1ecfdc7e235b9ada87fb7a71e455f7f
blktap2: Sort out tapdisk AIO init.

Move event callbacks registration into tapdisk-queue. This should also obsoletes the dummy pollfd pipe in the synchronous I/O case.

tools/blktap2/drivers/io-optimize.c +7 -0
diff -r aa0d1795f1ec -r 9746b010e2bf tools/blktap2/drivers/io-optimize.c
@@ -51,9 +51,16 @@
 opio_free(struct opioctx *ctx)
 {
 	free(ctx->opios);
+	ctx->opios = NULL;
+
 	free(ctx->free_opios);
+	ctx->free_opios = NULL;
+
 	free(ctx->iocb_queue);
+	ctx->iocb_queue = NULL;
+
 	free(ctx->event_queue);
+	ctx->event_queue = NULL;
 }
 
 int
tools/blktap2/drivers/qcow2raw.c +0 -6
diff -r aa0d1795f1ec -r 9746b010e2bf tools/blktap2/drivers/qcow2raw.c
@@ -101,12 +101,6 @@
     return;
 }
 
-void
-queue_event(event_id_t id, char mode, void *private)
-{
-  tapdisk_complete_tiocbs(&server.aio_queue);
-}
-
 static void debug_output(uint64_t progress, uint64_t size)
 {
         //Output progress every PROGRESS_QUANT 
tools/blktap2/drivers/tapdisk-queue.c +46 -19

120 lines not on this page · raw message

tools/blktap2/drivers/tapdisk-queue.h +2 -2
diff -r aa0d1795f1ec -r 9746b010e2bf tools/blktap2/drivers/tapdisk-queue.h
@@ -32,6 +32,7 @@
 #include <libaio.h>
 
 #include "io-optimize.h"
+#include "scheduler.h"
 
 struct tiocb;
 struct tfilter;
@@ -57,9 +58,9 @@
 	int                   sync;
 
 	int                   poll_fd;
+	event_id_t	      event;
 	io_context_t          aio_ctx;
 	struct opioctx        opioctx;
-	int                   dummy_pipe[2];
 
 	int                   queued;
 	struct iocb         **iocbs;
@@ -104,7 +105,6 @@
 void tapdisk_queue_tiocb(struct tqueue *, struct tiocb *);
 int tapdisk_submit_tiocbs(struct tqueue *);
 int tapdisk_submit_all_tiocbs(struct tqueue *);
-int tapdisk_complete_tiocbs(struct tqueue *);
 int tapdisk_cancel_tiocbs(struct tqueue *);
 int tapdisk_cancel_all_tiocbs(struct tqueue *);
 void tapdisk_prep_tiocb(struct tiocb *, int, int, char *, size_t,
tools/blktap2/drivers/tapdisk-server.c +9 -35

69 lines not on this page · raw message

tools/blktap2/drivers/tapdisk-server.h +0 -1
diff -r aa0d1795f1ec -r 9746b010e2bf tools/blktap2/drivers/tapdisk-server.h
@@ -58,7 +58,6 @@
 	struct list_head             vbds;
 	scheduler_t                  scheduler;
 	struct tqueue                aio_queue;
-	event_id_t                   aio_queue_event_id;
 } tapdisk_server_t;
 
 #endif

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

#9 lore raw

PATCH 1/1 blktap2: Separate tapdisk raw I/O into different backends

In reply to Daniel Stodden

# HG changeset patch
# User Daniel Stodden <[email protected]>
# Date 1264750223 28800
# Node ID 2b8c81d25096575472974f1e81829b73f0fe7d69 # Parent 9746b010e2bf495a1c93c37ab98f9d1fdde559df
blktap2: Separate tapdisk raw I/O into different backends.

Hide tapdisk support for different raw I/O interfaces behind a new struct tio. Libaio remains to dominate the interface, requiring everyone to dispatch iocb/ioevent structs.

Backends:
- lio: Kernel AIO via libaio.
- rwio: Canonical read/write() mode.

Misc:
- Fixes a bug in tapdisk-vbd which locks up the sync io mode.
- Wants a PERROR macro in blktaplib.h
- Removes dead code in qcow2raw to make it link again.

tools/blktap2/drivers/tapdisk-queue.c +283 -130
diff -r 9746b010e2bf -r 2b8c81d25096 tools/blktap2/drivers/tapdisk-queue.c
@@ -141,7 +141,7 @@
 	 * use a private linked list to keep track
 	 * of the tiocbs we're cancelling. 
 	 */
-	tiocb  = (struct tiocb *)queue->iocbs[0]->data;
+	tiocb  = queue->iocbs[0]->data;
 	queued = queue->queued;
 	queue->queued = 0;
 
@@ -165,8 +165,40 @@
 	return cancel_tiocbs(queue, err);
 }
 
+/*
+ * rwio
+ */
+
+struct rwio {
+	struct io_event *aio_events;
+};
+
+static void
+tapdisk_rwio_destroy(struct tqueue *queue)
+{
+	struct rwio *rwio = queue->tio_data;
+
+	if (rwio->aio_events) {
+		free(rwio->aio_events);
+		rwio->aio_events = NULL;
+	}
+}
+
+static int
+tapdisk_rwio_setup(struct tqueue *queue, int size)
+{
+	struct rwio *rwio = queue->tio_data;
+	int err;
+
+	rwio->aio_events = calloc(size, sizeof(struct io_event));
+	if (!rwio->aio_events)
+		return -errno;
+
+	return 0;
+}
+
 static inline ssize_t
-iocb_rw(struct iocb *iocb)
+tapdisk_rwio_rw(const struct iocb *iocb)
 {
 	int fd        = iocb->aio_fildes;
 	char *buf     = iocb->u.c.buf;
@@ -177,7 +209,7 @@
 
 	if (lseek(fd, off, SEEK_SET) == (off_t)-1)
 		return -errno;
-	
+
 	if (atomicio(func, fd, buf, size) != size)
 		return -errno;
 
@@ -185,8 +217,9 @@
 }
 
 static int
-io_synchronous_rw(struct tqueue *queue)
+tapdisk_rwio_submit(struct tqueue *queue)
 {
+	struct rwio *rwio = queue->tio_data;
 	int i, merged, split;
 	struct iocb *iocb;
 	struct tiocb *tiocb;
@@ -201,18 +234,18 @@
 	queue->queued = 0;
 
 	for (i = 0; i < merged; i++) {
-		ep      = queue->aio_events + i;
+		ep      = rwio->aio_events + i;
 		iocb    = queue->iocbs[i];
 		ep->obj = iocb;
-		ep->res = iocb_rw(iocb);
+		ep->res = tapdisk_rwio_rw(iocb);
 	}
 
-	split = io_split(&queue->opioctx, queue->aio_events, merged);
-	tapdisk_filter_events(queue->filter, queue->aio_events, split);
+	split = io_split(&queue->opioctx, rwio->aio_events, merged);
+	tapdisk_filter_events(queue->filter, rwio->aio_events, split);
 
-	for (i = split, ep = queue->aio_events; i-- > 0; ep++) {
+	for (i = split, ep = rwio->aio_events; i-- > 0; ep++) {
 		iocb  = ep->obj;
-		tiocb = (struct tiocb *)iocb->data;
+		tiocb = iocb->data;
 		complete_tiocb(queue, tiocb, ep->res);
 	}
 
@@ -221,65 +254,258 @@
 	return split;
 }
 
-static void tapdisk_tiocb_event(event_id_t id, char mode, void *private);
+static const struct tio td_tio_rwio = {
+	.name        = "rwio",
+	.data_size   = 0,
+	.tio_setup   = NULL,
+	.tio_destroy = NULL,
+	.tio_submit  = tapdisk_rwio_submit
+};
+
+/*
+ * libaio
+ */
+
+struct lio {
+	io_context_t     aio_ctx;
+	struct io_event *aio_events;
+
+	int              poll_fd;
+	int              event_id;
+};
+
+static void
+tapdisk_lio_destroy(struct tqueue *queue)
+{
+	struct lio *lio = queue->tio_data;
+
+	if (!lio)
+		return;
+
+	if (lio->event_id >= 0) {
+		tapdisk_server_unregister_event(lio->event_id);
+		lio->event_id = -1;
+	}
+
+	if (lio->aio_ctx) {
+		io_destroy(lio->aio_ctx);
+		lio->aio_ctx = NULL;
+	}
+
+	if (lio->aio_events) {
+		free(lio->aio_events);
+		lio->aio_events = NULL;
+	}
+}
+
+static void
+tapdisk_lio_event(event_id_t id, char mode, void *private)
+{
+	struct tqueue *queue = private;
+	struct lio *lio;
+	int i, ret, split;
+	struct iocb *iocb;
+	struct tiocb *tiocb;
+	struct io_event *ep;
+
+	lio   = queue->tio_data;
+	ret   = io_getevents(lio->aio_ctx, 0,
+			     queue->size, lio->aio_events, NULL);
+	split = io_split(&queue->opioctx, lio->aio_events, ret);
+	tapdisk_filter_events(queue->filter, lio->aio_events, split);
+
+	DBG("events: %d, tiocbs: %d\n", ret, split);
+
+	queue->iocbs_pending  -= ret;
+	queue->tiocbs_pending -= split;
+
+	for (i = split, ep = lio->aio_events; i-- > 0; ep++) {
+		iocb  = ep->obj;
+		tiocb = iocb->data;
+		complete_tiocb(queue, tiocb, ep->res);
+	}
+
+	queue_deferred_tiocbs(queue);
+}
+
+static int
+tapdisk_lio_setup(struct tqueue *queue, int qlen)
+{
+	struct lio *lio = queue->tio_data;
+	size_t sz;
+	int err;
+
+	lio->event_id = -1;
+	lio->aio_ctx  = REQUEST_ASYNC_FD;
+
+	lio->poll_fd = io_setup(qlen, &lio->aio_ctx);
+	err = lio->poll_fd;
+	if (err < 0) {
+		lio->aio_ctx = NULL;
+
+		if (err == -EAGAIN)
+			goto fail_rsv;
+
+		goto fail_fd;
+	}
+
+	lio->event_id =
+		tapdisk_server_register_event(SCHEDULER_POLL_READ_FD,
+					      lio->poll_fd, 0,
+					      tapdisk_lio_event,
+					      queue);
+	err = lio->event_id;
+	if (err < 0)
+		goto fail;
+
+	lio->aio_events = calloc(qlen, sizeof(struct io_event));
+	if (!lio->aio_events) {
+		err = -errno;
+		goto fail;
+	}
+
+	return 0;
+
+fail:
+	tapdisk_lio_destroy(queue);
+	return err;
+
+fail_rsv:
+	DPRINTF("Couldn't setup AIO context. If you are trying to "
+		"concurrently use a large number of blktap-based disks, you may "
+		"need to increase the system-wide aio request limit. "
+		"(e.g. 'echo 1048576 > /proc/sys/fs/aio-max-nr')\n");
+	goto fail;
+
+fail_fd:
+	DPRINTF("Couldn't get fd for AIO poll support. This is probably "
+		"because your kernel does not have  the aio-poll patch "
+		"applied.\n");
+	goto fail;
+}
+
+static int
+tapdisk_lio_submit(struct tqueue *queue)
+{
+	struct lio *lio = queue->tio_data;
+	int merged, submitted, err = 0;
+
+	if (!queue->queued)
+		return 0;
+
+	tapdisk_filter_iocbs(queue->filter, queue->iocbs, queue->queued);
+	merged    = io_merge(&queue->opioctx, queue->iocbs, queue->queued);
+	submitted = io_submit(lio->aio_ctx, merged, queue->iocbs);
+
+	DBG("queued: %d, merged: %d, submitted: %d\n",
+	    queue->queued, merged, submitted);
+
+	if (submitted < 0) {
+		err = submitted;
+		submitted = 0;
+	} else if (submitted < merged)
+		err = -EIO;
+
+	queue->iocbs_pending  += submitted;
+	queue->tiocbs_pending += queue->queued;
+	queue->queued          = 0;
+
+	if (err)
+		queue->tiocbs_pending -= 
+			fail_tiocbs(queue, submitted, merged, err);
+
+	return submitted;
+}
+
+static const struct tio td_tio_lio = {
+	.name        = "lio",
+	.data_size   = sizeof(struct lio),
+	.tio_setup   = tapdisk_lio_setup,
+	.tio_destroy = tapdisk_lio_destroy,
+	.tio_submit  = tapdisk_lio_submit,
+};
+
+static void
+tapdisk_queue_free_io(struct tqueue *queue)
+{
+	if (queue->tio) {
+		if (queue->tio->tio_destroy)
+			queue->tio->tio_destroy(queue);
+		queue->tio = NULL;
+	}
+
+	if (queue->tio_data) {
+		free(queue->tio_data);
+		queue->tio_data = NULL;
+	}
+}
+
+static int
+tapdisk_queue_init_io(struct tqueue *queue, int drv)
+{
+	const struct tio *tio;
+	int err;
+
+	switch (drv) {
+	case TIO_DRV_LIO:
+		tio = &td_tio_lio;
+		break;
+	case TIO_DRV_RWIO:
+		tio = &td_tio_rwio;
+		break;
+	default:
+		err = -EINVAL;
+		goto fail;
+	}
+
+	queue->tio_data = calloc(1, tio->data_size);
+	if (!queue->tio_data) {
+		PERROR("malloc(%zu)", tio->data_size);
+		err = -errno;
+		goto fail;
+	}
+
+	queue->tio = tio;
+
+	if (tio->tio_setup) {
+		err = tio->tio_setup(queue, queue->size);
+		if (err)
+			goto fail;
+	}
+
+	DPRINTF("I/O queue driver: %s\n", tio->name);
+
+	return 0;
+
+fail:
+	tapdisk_queue_free_io(queue);
+	return err;
+}
 
 int
 tapdisk_init_queue(struct tqueue *queue, int size,
-		   int sync, struct tfilter *filter)
+		   int drv, struct tfilter *filter)
 {
 	int i, err;
 
 	memset(queue, 0, sizeof(struct tqueue));
 
 	queue->size   = size;
-	queue->sync   = sync;
 	queue->filter = filter;
 
-	queue->event   = -1;
-	queue->aio_ctx = NULL;
-
 	if (!size)
 		return 0;
 
-	if (!sync) {
-		queue->aio_ctx = REQUEST_ASYNC_FD;
-		queue->poll_fd = io_setup(size, &queue->aio_ctx);
-		err = queue->poll_fd;
-		if (err < 0) {
-			if (err == -EAGAIN)
-				DPRINTF("Couldn't setup AIO context.  If you "
-					"are trying to concurrently use a "
-					"large number of blktap-based disks, "
-					"you may need to increase the "
-					"system-wide aio request limit. "
-					"(e.g. 'echo 1048576 > /proc/sys/fs/"
-					"aio-max-nr')\n");
-			else
-				DPRINTF("Couldn't get fd for AIO poll "
-					"support.  This is probably because "
-					"your kernel does not have the "
-					"aio-poll patch applied.\n");
-			queue->aio_ctx = NULL;
-			goto fail;
-		}
+	err = tapdisk_queue_init_io(queue, drv);
+	if (err)
+		goto fail;
 
-		queue->event =
-			tapdisk_server_register_event(SCHEDULER_POLL_READ_FD,
-						      queue->poll_fd, 0,
-						      tapdisk_tiocb_event,
-						      queue);
-		err = queue->event;
-		if (err < 0)
-			goto fail;
-
+	queue->iocbs = calloc(size, sizeof(struct iocb *));
+	if (!queue->iocbs) {
+		err = -errno;
+		goto fail;
 	}
 
-	err               = -ENOMEM;
-	queue->iocbs      = calloc(size, sizeof(struct iocb *));
-	queue->aio_events = calloc(size, sizeof(struct io_event));
-	if (!queue->iocbs || !queue->aio_events)
-		goto fail;
-
 	err = opio_init(&queue->opioctx, size);
 	if (err)
 		goto fail;
@@ -294,22 +520,11 @@
 void
 tapdisk_free_queue(struct tqueue *queue)
 {
-	if (queue->event >= 0) {
-		tapdisk_server_unregister_event(queue->event);
-		queue->event = -1;
-	}
-
-	if (queue->aio_ctx) {
-		io_destroy(queue->aio_ctx);
-		queue->aio_ctx = NULL;
-	}
+	tapdisk_queue_free_io(queue);
 
 	free(queue->iocbs);
 	queue->iocbs = NULL;
 
-	free(queue->aio_events);
-	queue->aio_events = NULL;
-
 	opio_free(&queue->opioctx);
 }
 
@@ -319,9 +534,9 @@
 	struct tiocb *tiocb = queue->deferred.head;
 
 	WARN("TAPDISK QUEUE:\n");
-	WARN("size: %d, sync: %d, queued: %d, iocbs_pending: %d, "
+	WARN("size: %d, tio: %s, queued: %d, iocbs_pending: %d, "
 	     "tiocbs_pending: %d, tiocbs_deferred: %d, deferrals: %"PRIx64"\n",
-	     queue->size, queue->sync, queue->queued, queue->iocbs_pending,
+	     queue->size, queue->tio->name, queue->queued, queue->iocbs_pending,
 	     queue->tiocbs_pending, queue->tiocbs_deferred, queue->deferrals);
 
 	if (tiocb) {
@@ -362,42 +577,14 @@
 		defer_tiocb(queue, tiocb);
 }
 
+
 /*
  * fail_tiocbs may queue more tiocbs
  */
 int
 tapdisk_submit_tiocbs(struct tqueue *queue)
 {
-	int merged, submitted, err = 0;
-
-	if (!queue->queued)
-		return 0;
-
-	if (queue->sync)
-		return io_synchronous_rw(queue);
-
-	tapdisk_filter_iocbs(queue->filter, queue->iocbs, queue->queued);
-	merged    = io_merge(&queue->opioctx, queue->iocbs, queue->queued);
-	submitted = io_submit(queue->aio_ctx, merged, queue->iocbs);
-
-	DBG("queued: %d, merged: %d, submitted: %d\n",
-	    queue->queued, merged, submitted);
-
-	if (submitted < 0) {
-		err = submitted;
-		submitted = 0;
-	} else if (submitted < merged)
-		err = -EIO;
-
-	queue->iocbs_pending  += submitted;
-	queue->tiocbs_pending += queue->queued;
-	queue->queued          = 0;
-
-	if (err)
-		queue->tiocbs_pending -= 
-			fail_tiocbs(queue, submitted, merged, err);
-
-	return submitted;
+	return queue->tio->tio_submit(queue);
 }
 
 int
@@ -412,40 +599,6 @@
 	return submitted;
 }
 
-static void
-tapdisk_complete_tiocbs(struct tqueue *queue)
-{
-	int i, ret, split;
-	struct iocb *iocb;
-	struct tiocb *tiocb;
-	struct io_event *ep;
-
-	ret   = io_getevents(queue->aio_ctx, 0,
-			     queue->size, queue->aio_events, NULL);
-	split = io_split(&queue->opioctx, queue->aio_events, ret);
-	tapdisk_filter_events(queue->filter, queue->aio_events, split);
-
-	DBG("events: %d, tiocbs: %d\n", ret, split);
-
-	queue->iocbs_pending  -= ret;
-	queue->tiocbs_pending -= split;
-
-	for (i = split, ep = queue->aio_events; i-- > 0; ep++) {
-		iocb  = ep->obj;
-		tiocb = (struct tiocb *)iocb->data;
-		complete_tiocb(queue, tiocb, ep->res);
-	}
-
-	queue_deferred_tiocbs(queue);
-}
-
-static void
-tapdisk_tiocb_event(event_id_t id, char mode, void *private)
-{
-	struct tqueue *queue = private;
-	tapdisk_complete_tiocbs(queue);
-}
-
 /*
  * cancel_tiocbs may queue more tiocbs
  */
tools/blktap2/drivers/tapdisk-queue.h +18 -6
diff -r 9746b010e2bf -r 2b8c81d25096 tools/blktap2/drivers/tapdisk-queue.h
@@ -55,16 +55,14 @@
 
 struct tqueue {
 	int                   size;
-	int                   sync;
 
-	int                   poll_fd;
-	event_id_t	      event;
-	io_context_t          aio_ctx;
+	const struct tio     *tio;
+	void                 *tio_data;
+
 	struct opioctx        opioctx;
 
 	int                   queued;
 	struct iocb         **iocbs;
-	struct io_event      *aio_events;
 
 	/* number of iocbs pending in the aio layer */
 	int                   iocbs_pending;
@@ -86,6 +84,20 @@
 	uint64_t              deferrals;
 };
 
+struct tio {
+	const char           *name;
+	size_t                data_size;
+
+	int  (*tio_setup)    (struct tqueue *queue, int qlen);
+	void (*tio_destroy)  (struct tqueue *queue);
+	int  (*tio_submit)   (struct tqueue *queue);
+};
+
+enum {
+	TIO_DRV_LIO     = 1,
+	TIO_DRV_RWIO    = 2,
+};
+
 /*
  * Interface for request producer (i.e., tapdisk)
  * NB: the following functions may cause additional tiocbs to be queued:
@@ -99,7 +111,7 @@
 #define tapdisk_queue_empty(q) ((q)->queued == 0)
 #define tapdisk_queue_full(q)  \
 	(((q)->tiocbs_pending + (q)->queued) >= (q)->size)
-int tapdisk_init_queue(struct tqueue *, int size, int sync, struct tfilter *);
+int tapdisk_init_queue(struct tqueue *, int size, int drv, struct tfilter *);
 void tapdisk_free_queue(struct tqueue *);
 void tapdisk_debug_queue(struct tqueue *);
 void tapdisk_queue_tiocb(struct tqueue *, struct tiocb *);
tools/blktap2/drivers/tapdisk-server.c +2 -1
diff -r 9746b010e2bf -r 2b8c81d25096 tools/blktap2/drivers/tapdisk-server.c
@@ -236,7 +236,8 @@
 static int
 tapdisk_server_init_aio(void)
 {
-	return tapdisk_init_queue(&server.aio_queue, TAPDISK_TIOCBS, 0, NULL);
+	return tapdisk_init_queue(&server.aio_queue, TAPDISK_TIOCBS,
+				  TIO_DRV_LIO, NULL);
 }
 
 static void
tools/blktap2/drivers/tapdisk-vbd.c +2 -0
diff -r 9746b010e2bf -r 2b8c81d25096 tools/blktap2/drivers/tapdisk-vbd.c
@@ -1260,6 +1260,8 @@
 	int n;
 	td_ring_t *ring;
 
+	tapdisk_vbd_check_state(vbd);
+
 	ring = &vbd->ring;
 	if (!ring->sring)
 		return 0;
tools/blktap2/include/blktaplib.h +1 -0
diff -r 9746b010e2bf -r 2b8c81d25096 tools/blktap2/include/blktaplib.h
@@ -43,6 +43,7 @@
 #endif
 
 #define EPRINTF(_f, _a...) syslog(LOG_ERR, "tap-err:%s: " _f, __func__, ##_a)
+#define PERROR(_f, _a...)  EPRINTF(_f ": %s", ##_a, strerror(errno))
 
 #define BLK_RING_SIZE __RING_SIZE((blkif_sring_t *)0, XC_PAGE_SIZE)
 

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

#10 lore raw

PATCH 1/1 blktap2: Prefer AIO eventfd support on kernels >= 2.6.22

In reply to Daniel Stodden

# HG changeset patch
# User Daniel Stodden <[email protected]>
# Date 1264750223 28800
# Node ID 1e60fec876f2254b62ebc052fe4eb3ce96c016d4 # Parent 2b8c81d25096575472974f1e81829b73f0fe7d69
blktap2: Prefer AIO eventfd support on kernels >= 2.6.22

Mainline kernel support for eventfd(2) in linux aio was added between 2.6.21 and 2.6.22. Libaio after 0.3.107 has the header file, but presently few systems support it. Neither do we rely on an up-to-date libc6.

Instead, this patch adds a header which defines custom iocb_common struct, and works around a potentially missing sys/eventfd.h.

tools/blktap2/drivers/block-aio.c +0 -1
diff -r 2b8c81d25096 -r 1e60fec876f2 tools/blktap2/drivers/block-aio.c
@@ -28,7 +28,6 @@
 
 
 #include <errno.h>
-#include <libaio.h>
 #include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
tools/blktap2/drivers/libaio-compat.h new file +92 -0

92 lines not on this page · raw message

tools/blktap2/drivers/tapdisk-queue.c +148 -30

239 lines not on this page · raw message

tools/blktap2/drivers/tapdisk-utils.c +32 -0
diff -r 2b8c81d25096 -r 1e60fec876f2 tools/blktap2/drivers/tapdisk-utils.c
@@ -33,6 +33,10 @@
 #include <sys/mman.h>
 #include <sys/ioctl.h>
 #include <sys/resource.h>
+#include <sys/utsname.h>
+#ifdef __linux__
+#include <linux/version.h>
+#endif
 
 #include "blk.h"
 #include "tapdisk.h"
@@ -183,3 +187,31 @@
 
 	return 0;
 }
+
+#ifdef __linux__
+
+int tapdisk_linux_version(void)
+{
+	struct utsname uts;
+	unsigned int version, patchlevel, sublevel;
+	int n, err;
+
+	err = uname(&uts);
+	if (err)
+		return -errno;
+
+	n = sscanf(uts.release, "%u.%u.%u", &version, &patchlevel, &sublevel);
+	if (n != 3)
+		return -ENOSYS;
+
+	return KERNEL_VERSION(version, patchlevel, sublevel);
+}
+
+#else
+
+int tapdisk_linux_version(void)
+{
+	return -ENOSYS;
+}
+
+#endif
tools/blktap2/drivers/tapdisk-utils.h +1 -0
diff -r 2b8c81d25096 -r 1e60fec876f2 tools/blktap2/drivers/tapdisk-utils.h
@@ -38,5 +38,6 @@
 int tapdisk_namedup(char **, const char *);
 int tapdisk_parse_disk_type(const char *, char **, int *);
 int tapdisk_get_image_size(int, uint64_t *, uint32_t *);
+int tapdisk_linux_version(void);
 
 #endif

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

#13 lore raw
Quoted 3 lines · Keir Fraser

Echo the changeset comment and sign-off into the email body would be better, but your re-send is fine as far as I'm concerned. I should be able to apply the attachments no problem.

Patchbomb as of hg 1.3.1 doesn't seem to do this.

Xapi-dev promotes a similar patchbomb hack, but it would include the entire diff in the header message (--review) and a dup attached to avoid mangling.

Does one exist already, or are you somewhat asking me to write one? Would probably belong on the wiki.

Daniel

#14 lore raw
Quoted 6 lines · Daniel Stodden
Quoted 3 lines · Keir Fraser

Echo the changeset comment and sign-off into the email body would be better, but your re-send is fine as far as I'm concerned. I should be able to apply the attachments no problem.

Patchbomb as of hg 1.3.1 doesn't seem to do this.

Ah, it does.

So --inline implies --attach,
but is still different from --inline *and* --attach.

Sorry for the noise.

:P,
Daniel

#15 lore raw
Quoted 6 lines · Daniel Stodden
Quoted 5 lines · Daniel Stodden
Quoted 3 lines · Keir Fraser

Echo the changeset comment and sign-off into the email body would be better, but your re-send is fine as far as I'm concerned. I should be able to apply the attachments no problem.

Quoted 8 lines
Quoted 1 line

Patchbomb as of hg 1.3.1 doesn't seem to do this.

Ah, it does.

So --inline implies --attach,
but is still different from --inline *and* --attach.

Sorry for the noise.

By the way, attaching and inlining a whole patch is also acceptable. Some people like inline patches, so that they can easily review and comment. I like attachments because they JustWork when I'm trying to apply big bundles of patches.

For smaller patches, inline only can be okay. For some reason, I find that inline large patches rarely apply for me. It's really weird, given I only fetchmail and munpack the emails, and yet I have a suspicion the mangling does happen at my end.

-- Keir

#16 lore raw
Quoted 15 lines · Keir Fraser
Quoted 13 lines · Daniel Stodden
Quoted 6 lines · Daniel Stodden
Quoted 3 lines · Keir Fraser

Echo the changeset comment and sign-off into the email body would be better, but your re-send is fine as far as I'm concerned. I should be able to apply the attachments no problem.

Patchbomb as of hg 1.3.1 doesn't seem to do this.

Ah, it does.

So --inline implies --attach,
but is still different from --inline *and* --attach.

Quoted 6 lines
Quoted 1 line

Sorry for the noise.

By the way, attaching and inlining a whole patch is also acceptable. Some people like inline patches, so that they can easily review and comment. I like attachments because they JustWork when I'm trying to apply big bundles of patches.

To avoid confusion: By inline I meant the content-disposition. The visual results would combine the best of both. But I suspect the results somwhat depend on the mailer in use.

It's completely up to you. I don't charge for python cycles. :)

So far I got

[alias]
email-xen = email --attach --inline --to 'Xen <[email protected]>' email-xapi = email --review --inline --to 'Xen API <[email protected]>'

Maybe Pasi knows a decent corner on the wiki where this stuff can converge to gatekeeper preferences.

Quoted 4 lines

For smaller patches, inline only can be okay. For some reason, I find that inline large patches rarely apply for me. It's really weird, given I only fetchmail and munpack the emails, and yet I have a suspicion the mangling does happen at my end.

Heard this before.

I doubt fetchmail tries to be smart about the message body.

I never worked with munpack.

I noticed that mailman adds some filtering even when forwarding, because the content-disposition=inline gets stripped. But I also think mailman breaking diffs would be kind of gross..

Cheers,
Daniel

#17 lore raw
Quoted 29 lines · Daniel Stodden
Quoted 15 lines · Keir Fraser
Quoted 13 lines · Daniel Stodden
Quoted 6 lines · Daniel Stodden
Quoted 3 lines · Keir Fraser

Echo the changeset comment and sign-off into the email body would be better, but your re-send is fine as far as I'm concerned. I should be able to apply the attachments no problem.

Patchbomb as of hg 1.3.1 doesn't seem to do this.

Ah, it does.

So --inline implies --attach,
but is still different from --inline *and* --attach.

Quoted 6 lines
Quoted 1 line

Sorry for the noise.

By the way, attaching and inlining a whole patch is also acceptable. Some people like inline patches, so that they can easily review and comment. I like attachments because they JustWork when I'm trying to apply big bundles of patches.

To avoid confusion: By inline I meant the content-disposition. The visual results would combine the best of both. But I suspect the results somwhat depend on the mailer in use.

It's completely up to you. I don't charge for python cycles. :)

Quoted 9 lines

So far I got

[alias]
email-xen = email --attach --inline --to 'Xen <[email protected]>' email-xapi = email --review --inline --to 'Xen API <[email protected]>'

Maybe Pasi knows a decent corner on the wiki where this stuff can converge to gatekeeper preferences.

There's a note about submitting patches in:
http://wiki.xensource.com/xenwiki/XenFaq

I can add this stuff there.

So those aliases should go to .hgrc ?

-- Pasi

#18 lore raw
Quoted 40 lines · Pasi Kärkkäinen
Quoted 29 lines · Daniel Stodden
Quoted 15 lines · Keir Fraser
Quoted 13 lines · Daniel Stodden
Quoted 6 lines · Daniel Stodden
Quoted 3 lines · Keir Fraser

Echo the changeset comment and sign-off into the email body would be better, but your re-send is fine as far as I'm concerned. I should be able to apply the attachments no problem.

Patchbomb as of hg 1.3.1 doesn't seem to do this.

Ah, it does.

So --inline implies --attach,
but is still different from --inline *and* --attach.

Quoted 6 lines
Quoted 1 line

Sorry for the noise.

By the way, attaching and inlining a whole patch is also acceptable. Some people like inline patches, so that they can easily review and comment. I like attachments because they JustWork when I'm trying to apply big bundles of patches.

To avoid confusion: By inline I meant the content-disposition. The visual results would combine the best of both. But I suspect the results somwhat depend on the mailer in use.

It's completely up to you. I don't charge for python cycles. :)

Quoted 9 lines

So far I got

[alias]
email-xen = email --attach --inline --to 'Xen <[email protected]>' email-xapi = email --review --inline --to 'Xen API <[email protected]>'

Maybe Pasi knows a decent corner on the wiki where this stuff can converge to gatekeeper preferences.

Quoted 6 lines

There's a note about submitting patches in:
http://wiki.xensource.com/xenwiki/XenFaq

I can add this stuff there.

So those aliases should go to .hgrc ?

May. It's just a neat way to not memorize them, so I guess it might help.

I remember a xensource wiki page where the preferred format for xapi-dev was documented. I send you a copy when I'm back at work.

That took a patch to patchbomb though. I pasted the one I'm presently running below.

Not sure: I think there's already a fair number xen-devel people looking into XCP. But is Xapi-related stuff even wanted on the current wiki?

Daniel

patchbomb.py Fri Jan 29 00:49:13 2010 -0800 renamed from patchbomb.py Fri Jan 29 00:20:30 2010 -0800 +4 -0
diff -r 13e59a0b3485 -r c60348153509 patchbomb.py
@@ -126,6 +126,9 @@
                 'Patch subject is complete summary.')
         body += '\n\n\n'
 
+    if opts.get('review'):
+        body += '\n'.join(patch)
+
     if opts.get('plain'):
         while patch and patch[0].startswith('# '):
             patch.pop(0)
@@ -494,6 +497,7 @@
           ('', 'bundlename', 'bundle',
            _('name of the bundle attachment file')),
           ('r', 'rev', [], _('a revision to send')),
+          ('R', 'review', None, _('add patch to message body for review')),
           ('', 'force', None,
            _('run even when remote repository is unrelated '
              '(with -b/--bundle)')),
#19 lore raw
In reply to Daniel Stodden
Quoted 2 lines · Daniel Stodden

- Does a runtime kernel version check. I guess this code will need additional cpp magic on BSDs.

I guess BSD will need cpp magic regardless of the kernel version check since it will have a different interface altogether. But for the Linux version can we not just try eventfd(), check for -ENOSYS and fall back to the old way if necessary, instead of hardcoding the kernel version?

I suppose eventfd is an unlikely candidate for backporting to anything older but in general IMHO it's preferable to check for the actual functionality you want to use rather than hardcoding version numbers etc.

Ian.

#20 lore raw
Quoted 6 lines · Ian Campbell
Quoted 2 lines · Daniel Stodden

- Does a runtime kernel version check. I guess this code will need additional cpp magic on BSDs.

I guess BSD will need cpp magic regardless of the kernel version check since it will have a different interface altogether.

Right now the version check is only done ifdef __linux__, or it falls back to the poll patch right away. Whatever BSD is precisely running there, I didn't change much about the ABI tapdisk is using.

Quoted 3 lines

But for the Linux
version can we not just try eventfd(), check for -ENOSYS and fall back to the old way if necessary, instead of hardcoding the kernel version?

That was my initial thought.
Then again eventfd went in before AIO caught up.
Then again-again it's just a minor number in between. It's probably fair to assume that the number of dom0 kernels out there which are prone to surprises is quite limited.

Quoted 4 lines

I suppose eventfd is an unlikely candidate for backporting to anything older but in general IMHO it's preferable to check for the actual functionality you want to use rather than hardcoding version numbers etc.

In theory I agree.

I guess the right (tm) way to fix is to convince the libaio maintainer to add a proper version define somewhere, instead of doing it in tapdisk. Then let distros fix libaio if they want it. Which they most probably want anyway.

The objective was to do without for now, and obsolete tools/libaio asap.

The uname() was just to guarantee the least possible amount of unwanted surprise while doing so.

If the gap is a non-issue, let's remove that bit.
I'm all for smaller code.

Daniel

#21 lore raw
In reply to Daniel Stodden

Is this patch set related to the AIO poll support that blocktap2 keeps complaining about when I use tap:tapdisk:aio as the disk type? I found the code that changes io_setup in the Novell Xen patches but its part of a reasonably large patch.

Dave

#22 lore raw
Quoted 2 lines · David P. Quigley

Is this patch set related to the AIO poll support that blocktap2 keeps complaining about when I use tap:tapdisk:aio as the disk type?

On pvops, yes.

Quoted 3 lines

I found
the code that changes io_setup in the Novell Xen patches but its part of a reasonably large patch.

Shouldn't depend on the disktype. As long as it's blktap, not blkback on a raw vdi, all disk drivers depend on this.

The io_setup change you're referring might rather be the old way of doing things. It made io_setup return an fd to poll, if the io_setup caller calls with a 'magic' (1, actually) aio_ctx value.

Daniel

#23 lore raw
Quoted 10 lines · Daniel Stodden
Quoted 2 lines · David P. Quigley

Is this patch set related to the AIO poll support that blocktap2 keeps complaining about when I use tap:tapdisk:aio as the disk type?

On pvops, yes.

Quoted 3 lines

I found
the code that changes io_setup in the Novell Xen patches but its part of a reasonably large patch.

Quoted 9 lines

Shouldn't depend on the disktype. As long as it's blktap, not blkback on a raw vdi, all disk drivers depend on this.

The io_setup change you're referring might rather be the old way of doing things. It made io_setup return an fd to poll, if the io_setup caller calls with a 'magic' (1, actually) aio_ctx value.

Daniel

I believe that is the case. The error message below is what I'm getting from tapdisk2. The ioctl successfully completes and the devices are created. However it just can't grab the fd to use so it then uses a second ioctl to remove the device shortly after.

Dave

Feb  1 17:29:20 localhost tapdisk2[2798]: Created /dev/xen/blktap-2/control device
Feb  1 17:29:20 localhost tapdisk2[2798]: Created /dev/xen/blktap-2/blktap0 device
Feb  1 17:29:20 localhost tapdisk2[2798]: Created /dev/xen/blktap-2/tapdev0 device
Feb  1 17:29:20 localhost tapdisk2[2798]: new interface: ring: 251, device: 253, minor: 0
Feb  1 17:29:20 localhost tapdisk2[2798]: Couldn't get fd for AIO poll support.  This is probably because your kernel does not have the aio-poll patch applied.
#24 lore raw
In reply to Daniel Stodden
Quoted 10 lines · Daniel Stodden
Quoted 2 lines · David P. Quigley

Is this patch set related to the AIO poll support that blocktap2 keeps complaining about when I use tap:tapdisk:aio as the disk type?

On pvops, yes.

Quoted 3 lines

I found
the code that changes io_setup in the Novell Xen patches but its part of a reasonably large patch.

Quoted 9 lines

Shouldn't depend on the disktype. As long as it's blktap, not blkback on a raw vdi, all disk drivers depend on this.

The io_setup change you're referring might rather be the old way of doing things. It made io_setup return an fd to poll, if the io_setup caller calls with a 'magic' (1, actually) aio_ctx value.

Daniel

Seems like I made a mistake and copied the version of the error that was generated before I updated xen-unstable to have your latest changes. Now I'm getting a completely different error. Does the aio driver handle raw disk files or do I need to create the image in some special way? Not the error I get has to do with incorrect magic in the shared info and this results in a segfault. I'll try to hunt them down. The tapdisk output is listed below.

Dave

tapdisk2[2366]: Created /dev/xen/blktap-2/blktap0 device
tapdisk2[2366]: Created /dev/xen/blktap-2/tapdev0 device
tapdisk2[2366]: new interface: ring: 251, device: 253, minor: 0
tapdisk2[2366]: I/O queue driver: lio
tapdisk2[2366]: Incorrect magic in shared info.
tapdisk2[2366]: Failed to open shared info.
tapdisk2[2366]: segfault at 18 ip 0000003e42008dc1 sp 00007fffd29889a0 error 4 in libpthread-2.10.2.so[3e42000000+17000]

#25 lore raw
Quoted 31 lines · David P. Quigley
Quoted 10 lines · Daniel Stodden
Quoted 2 lines · David P. Quigley

Is this patch set related to the AIO poll support that blocktap2 keeps complaining about when I use tap:tapdisk:aio as the disk type?

On pvops, yes.

Quoted 3 lines

I found
the code that changes io_setup in the Novell Xen patches but its part of a reasonably large patch.

Quoted 9 lines

Shouldn't depend on the disktype. As long as it's blktap, not blkback on a raw vdi, all disk drivers depend on this.

The io_setup change you're referring might rather be the old way of doing things. It made io_setup return an fd to poll, if the io_setup caller calls with a 'magic' (1, actually) aio_ctx value.

Daniel

Seems like I made a mistake and copied the version of the error that was generated before I updated xen-unstable to have your latest changes. Now I'm getting a completely different error. Does the aio driver handle raw disk files or do I need to create the image in some special way? Not the error I get has to do with incorrect magic in the shared info and this results in a segfault. I'll try to hunt them down. The tapdisk output is listed below.

Dave

Quoted 7 lines

tapdisk2[2366]: Created /dev/xen/blktap-2/blktap0 device
tapdisk2[2366]: Created /dev/xen/blktap-2/tapdev0 device
tapdisk2[2366]: new interface: ring: 251, device: 253, minor: 0
tapdisk2[2366]: I/O queue driver: lio
tapdisk2[2366]: Incorrect magic in shared info.
tapdisk2[2366]: Failed to open shared info.
tapdisk2[2366]: segfault at 18 ip 0000003e42008dc1 sp 00007fffd29889a0 error 4 in libpthread-2.10.2.so[3e42000000+17000]

Please try disabling -DMEMSHR in the Makefile.

[Assuming that helps: Keir, I think there is help underway, but should we turn it off by default in the meantime?]

Is this on the pvops kernel?

- Make sure it doesn't have HIGHPTE set.
- Apply the bunch of kernel patches posted. ("blktap2 on pvops updates") I don't think they made it into Jeremy's tree yet.

Let me know if there's more trouble, we're presently preparing another bunch of updates, so the time is just right :)

Daniel

#26 lore raw
Quoted 47 lines · Daniel Stodden
Quoted 31 lines · David P. Quigley
Quoted 10 lines · Daniel Stodden
Quoted 2 lines · David P. Quigley

Is this patch set related to the AIO poll support that blocktap2 keeps complaining about when I use tap:tapdisk:aio as the disk type?

On pvops, yes.

Quoted 3 lines

I found
the code that changes io_setup in the Novell Xen patches but its part of a reasonably large patch.

Quoted 9 lines

Shouldn't depend on the disktype. As long as it's blktap, not blkback on a raw vdi, all disk drivers depend on this.

The io_setup change you're referring might rather be the old way of doing things. It made io_setup return an fd to poll, if the io_setup caller calls with a 'magic' (1, actually) aio_ctx value.

Daniel

Seems like I made a mistake and copied the version of the error that was generated before I updated xen-unstable to have your latest changes. Now I'm getting a completely different error. Does the aio driver handle raw disk files or do I need to create the image in some special way? Not the error I get has to do with incorrect magic in the shared info and this results in a segfault. I'll try to hunt them down. The tapdisk output is listed below.

Dave

Quoted 7 lines

tapdisk2[2366]: Created /dev/xen/blktap-2/blktap0 device
tapdisk2[2366]: Created /dev/xen/blktap-2/tapdev0 device
tapdisk2[2366]: new interface: ring: 251, device: 253, minor: 0
tapdisk2[2366]: I/O queue driver: lio
tapdisk2[2366]: Incorrect magic in shared info.
tapdisk2[2366]: Failed to open shared info.
tapdisk2[2366]: segfault at 18 ip 0000003e42008dc1 sp 00007fffd29889a0 error 4 in libpthread-2.10.2.so[3e42000000+17000]

Please try disabling -DMEMSHR in the Makefile.

[Assuming that helps: Keir, I think there is help underway, but should we turn it off by default in the meantime?]

Is this on the pvops kernel?

Quoted 8 lines

- Make sure it doesn't have HIGHPTE set.
- Apply the bunch of kernel patches posted. ("blktap2 on pvops updates") I don't think they made it into Jeremy's tree yet.

Let me know if there's more trouble, we're presently preparing another bunch of updates, so the time is just right :)

Daniel

So I'm applying the patches and building now so I haven't have any trouble yet but you have a small issue with your 4th patch in the series you posted late last week. I actually looked into making blktap2 compile as a module and I ran into the two symbols you exported. I don't see a way around exporting zap_page_range however mm_init isn't supposed to be used anymore. If you look in Documentation/vm/active_mm.txt there is this text below.

[Start Quote]
Also, a new rule is that _nobody_ ever has "init_mm" as a real MM any more. "init_mm" should be considered just a "lazy context when no other context is available", and in fact it is mainly used just at bootup when no real VM has yet been created. So code that used to check

        if (current->mm == &init_mm)

should generally just do

        if (!current->mm)

instead (which makes more sense anyway - the test is basically one of "do we have a user context", and is generally done by the page fault handler and things like that).

[End Quote]

I'm not sure if you really don't have a real context at that point but I found it weird that init_mm was being used when this documentation seems to indicate no one should really be using it.

Dave

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

#29 lore raw

Hmm. DEBUG_SG set, I didn't try that yet, but I didn't expect issues either. Let's see if I manage a repro.

Daniel

Quoted 67 lines · David P. Quigley
------------[ cut here ]------------
kernel BUG at /home/dpquigl/linux-2.6-pvops.git/include/linux/scatterlist.h:65!
invalid opcode: 0000 [#1] SMP 
last sysfs file: /sys/devices/virtual/block/tapdevc/removable
CPU 1 
Modules linked in: nfs fscache ipt_MASQUERADE iptable_nat nf_nat nfsd bridge lockd stp nfs_acl llc auth_rpcgss bnep exportfs sco l2cap bluetooth rfkill sunrpc ip6t_REJECT nf_conntrack_ipv6 ip6table_filter ip6_tables ipv6 dm_multipath uinput snd_hda_codec_analog snd_hda_intel snd_hda_codec radeon snd_hwdep snd_seq ttm snd_seq_device drm snd_pcm snd_timer snd ppdev ata_generic soundcore e1000e dcdbas parport_pc i2c_algo_bit i2c_i801 snd_page_alloc iTCO_wdt serio_raw joydev parport pcspkr pata_acpi wmi iTCO_vendor_support i2c_core [last unloaded: microcode]
Pid: 17105, comm: vol_id Not tainted 2.6.31.6-pvops-dom0 #20 OptiPlex 960                 
RIP: e030:[<ffffffff81267d75>]  [<ffffffff81267d75>] sg_set_page+0x35/0x67
RSP: e02b:ffff8801bf03f538  EFLAGS: 00010293
RAX: 0000000000000000 RBX: ffff8801c482e7a0 RCX: 0000000000000000
RDX: 0000000000001000 RSI: ffffea000b7dec40 RDI: ffff8801c482e7a0
RBP: ffff8801bf03f548 R08: 0000000087654321 R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000000 R12: ffff8801bf066470
R13: ffff8801bf078000 R14: ffff8801ddd4f320 R15: ffff8801bf066400
FS:  00007f1cc9fdf6f0(0000) GS:ffffc900001d9000(0000) knlGS:0000000000000000
CS:  e033 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000001cf75a8 CR3: 00000001ca1df000 CR4: 0000000000002660
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process vol_id (pid: 17105, threadinfo ffff8801bf03e000, task ffff8801c650c880)
Stack:
 ffff8801bf03f558 000000000e73a217 ffff8801bf03f5c8 ffffffff8126867d
<0> ffff8801bf03f588 ffffffff8108e686 0000000000000000 000000000173a217
<0> ffff8801c482e7a0 0000000000000000 ffff8801bf03f5c8 000000000e73a217
Call Trace:
 [<ffffffff8126867d>] blk_rq_map_sg+0x1c8/0x2ff
 [<ffffffff8108e686>] ? getnstimeofday+0x6e/0xd8
 [<ffffffff81313d73>] blktap_device_process_request+0xb7/0x973
 [<ffffffff81133d26>] ? check_object+0x184/0x1d3
 [<ffffffff8100f61f>] ? xen_restore_fl_direct_end+0x0/0x1
 [<ffffffff8109aab3>] ? lock_acquired+0x2b4/0x2d7
 [<ffffffff8100f61f>] ? xen_restore_fl_direct_end+0x0/0x1
 [<ffffffff813148b3>] ? blktap_device_do_request+0x1e7/0x2ca
 [<ffffffff813148b3>] ? blktap_device_do_request+0x1e7/0x2ca
 [<ffffffff813148c1>] blktap_device_do_request+0x1f5/0x2ca
 [<ffffffff81074b70>] ? del_timer+0x63/0x85
 [<ffffffff810fb538>] ? sync_page_killable+0x0/0x5e
 [<ffffffff81261134>] __generic_unplug_device+0x44/0x5f
 [<ffffffff8100f632>] ? check_events+0x12/0x20
 [<ffffffff8126118c>] generic_unplug_device+0x3d/0x64
 [<ffffffff8100eb68>] ? xen_force_evtchn_callback+0x20/0x36
 [<ffffffff8125dfdd>] blk_unplug+0x38/0x53
 [<ffffffff8125e01d>] blk_backing_dev_unplug+0x25/0x3b
 [<ffffffff8108430a>] ? prepare_to_wait_exclusive+0x34/0x8b
 [<ffffffff811682ce>] block_sync_page+0x47/0x62
 [<ffffffff810fb51b>] sync_page+0x5f/0x7c
 [<ffffffff8100f632>] ? check_events+0x12/0x20
 [<ffffffff810fb559>] sync_page_killable+0x21/0x5e
 [<ffffffff8151365c>] __wait_on_bit_lock+0x55/0xb2
 [<ffffffff8100f61f>] ? xen_restore_fl_direct_end+0x0/0x1
 [<ffffffff810fb310>] __lock_page_killable+0x73/0x8e
 [<ffffffff81084132>] ? wake_bit_function+0x0/0x5a
 [<ffffffff810fb376>] lock_page_killable+0x4b/0x66
 [<ffffffff810fd596>] generic_file_aio_read+0x385/0x52f
 [<ffffffff81141ed4>] do_sync_read+0xfa/0x14b
 [<ffffffff8100eb68>] ? xen_force_evtchn_callback+0x20/0x36
 [<ffffffff810840d3>] ? autoremove_wake_function+0x0/0x5f
 [<ffffffff81225eec>] ? security_file_permission+0x29/0x3f
 [<ffffffff81142692>] vfs_read+0xba/0x12b
 [<ffffffff811427f5>] sys_read+0x59/0x91
 [<ffffffff81014fb2>] system_call_fastpath+0x16/0x1b
Code: 44 00 00 65 48 8b 04 25 28 00 00 00 48 89 45 f8 31 c0 40 f6 c6 03 48 8b 47 08 74 04 0f 0b eb fe 41 b8 21 43 65 87 4c 39 07 74 04 <0f> 0b eb fe a8 01 74 04 0f 0b eb fe 83 e0 03 89 4f 10 89 57 14 
RIP  [<ffffffff81267d75>] sg_set_page+0x35/0x67
 RSP <ffff8801bf03f538>
---[ end trace c72cd6244d0817d8 ]---
#30 lore raw
Quoted 2 lines · Daniel Stodden

Hmm. DEBUG_SG set, I didn't try that yet, but I didn't expect issues either. Let's see if I manage a repro.

Apart from that, thought you might later be willing to give the one below a spin.

Daniel

blktap2: Remap kernel address ranges through the tapdisk mm.

Avoid init_mm, which isn't exported to modules anymore.

drivers/xen/blktap/device.c +2 -2
diff -r e387b1974af5 drivers/xen/blktap/device.c
@@ -323,7 +323,7 @@
 		      request->handles[i].user);
 
 		if (request->handles[i].kernel == INVALID_GRANT_HANDLE) {
-			blktap_umap_uaddr(&init_mm, kvaddr);
+			blktap_umap_uaddr(tap->ring.vma->vm_mm, kvaddr);
 			flush_tlb_kernel_page(kvaddr);
 			set_phys_to_machine(__pa(kvaddr) >> PAGE_SHIFT,
 					    INVALID_P2M_ENTRY);
@@ -561,7 +561,7 @@
 	pte = mk_pte(page, ring->vma->vm_page_prot);
 	blktap_map_uaddr(ring->vma->vm_mm, uvaddr, pte_mkwrite(pte));
 	flush_tlb_page(ring->vma, uvaddr);
-	blktap_map_uaddr(&init_mm, kvaddr, mk_pte(page, PAGE_KERNEL));
+	blktap_map_uaddr(ring->vma->vm_mm, kvaddr, mk_pte(page, PAGE_KERNEL));
 	flush_tlb_kernel_page(kvaddr);
 
 	set_phys_to_machine(__pa(kvaddr) >> PAGE_SHIFT, pte_mfn(pte));
mm/init-mm.c +0 -2
diff -r e387b1974af5 mm/init-mm.c
@@ -4,7 +4,6 @@
 #include <linux/spinlock.h>
 #include <linux/list.h>
 #include <linux/cpumask.h>
-#include <linux/module.h>
 
 #include <asm/atomic.h>
 #include <asm/pgtable.h>
@@ -19,4 +18,3 @@
 	.mmlist		= LIST_HEAD_INIT(init_mm.mmlist),
 	.cpu_vm_mask	= CPU_MASK_ALL,
 };
-EXPORT_SYMBOL_GPL(init_mm);

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

#31 lore raw
Quoted 9 lines · Daniel Stodden
Quoted 2 lines · Daniel Stodden

Hmm. DEBUG_SG set, I didn't try that yet, but I didn't expect issues either. Let's see if I manage a repro.

Apart from that, thought you might later be willing to give the one below a spin.

Daniel

I'll add it to my guilt patchset and give it a try. Should I turn off DEBUG_SG in my config?

Dave

#32 lore raw
Quoted 5 lines · David P. Quigley
Quoted 4 lines · Daniel Stodden
Quoted 2 lines · Daniel Stodden

Hmm. DEBUG_SG set, I didn't try that yet, but I didn't expect issues either. Let's see if I manage a repro.

Quoted 8 lines
Quoted 5 lines

Apart from that, thought you might later be willing to give the one below a spin.

Daniel

I'll add it to my guilt patchset and give it a try. Should I turn off DEBUG_SG in my config?

Nooo, that's all great, let's rather see what else you dig up. Put the patch below on top, it worked for me.
I'll fold them later and post an update.

Daniel

drivers/xen/blktap/control.c +1 -0
diff -r c800f72f0a7d -r 7d95498e6a7b drivers/xen/blktap/control.c
@@ -23,6 +23,7 @@
 	init_rwsem(&tap->tap_sem);
 	init_waitqueue_head(&tap->wq);
 	atomic_set(&tap->refcnt, 0);
+	sg_init_table(tap->sg, BLKIF_MAX_SEGMENTS_PER_REQUEST);
 
 	tap->minor = minor;
 }

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

#33 lore raw
Quoted 12 lines · Daniel Stodden
Quoted 11 lines · David P. Quigley
Quoted 9 lines · Daniel Stodden
Quoted 2 lines · Daniel Stodden

Hmm. DEBUG_SG set, I didn't try that yet, but I didn't expect issues either. Let's see if I manage a repro.

Apart from that, thought you might later be willing to give the one below a spin.

Daniel

Quoted 8 lines
Quoted 2 lines

I'll add it to my guilt patchset and give it a try. Should I turn off DEBUG_SG in my config?

Nooo, that's all great, let's rather see what else you dig up. Put the patch below on top, it worked for me.
I'll fold them later and post an update.

Daniel

I still have the same oops even after the init-mm patch. Not quite sure whats causing it but the domain gets built even though I get the kernel oops with blktap. Unfortunately the vm then hangs as it has no disk to use for booting.

Dave