[PATCH libaio] add timeout to io_queue_run and remove io_queue_wait

[PATCH libaio] add timeout to io_queue_run and remove io_queue_wait

[Daniel McNeil]

[-- Attachment #1: Type: text/plain, Size: 644 bytes --]

Here is a patch to libaio which adds a timeout parameter to
io_queue_run() and removes io_queue_wait().  This makes my
earlier kernel patch to fs/aio.c unnecessary, since I removed
io_queue_wait() which was calling the io_getevents() syscall
with a zero number of events.  This keeps the callback interface
in the library and allows a user process to wait for i/o's to
complete if it wants to.  I also changed io_queue_run() to
batch up to 8 completions at a time, so it is more efficient.
This requires no kernel changes. I also updated the io_queue_run() 
man page and that is in a separate patch.

Thoughts?

Daniel McNeil <daniel@osdl.org


[-- Attachment #2: patch.libaio.io_queue_run --]
[-- Type: text/x-patch, Size: 3043 bytes --]

diff -rupN -X /home/daniel_nfs/dontdiff libaio-0.3.93/src/io_queue_run.c libaio-0.3.93-io_queue_run_with_timeout/src/io_queue_run.c
--- libaio-0.3.93/src/io_queue_run.c	2002-09-26 09:39:38.000000000 -0700
+++ libaio-0.3.93-io_queue_run_with_timeout/src/io_queue_run.c	2003-07-08 16:38:52.983739711 -0700
@@ -21,19 +21,33 @@
 #include <stdlib.h>
 #include <time.h>
 
-int io_queue_run(io_context_t ctx)
+#define IO_BATCH_EVENTS	8		/* number of events to batch up */
+
+int io_queue_run(io_context_t ctx, struct timespec *timeout)
 {
-	static struct timespec timeout = { 0, 0 };
-	struct io_event event;
-	int ret;
-
-	/* FIXME: batch requests? */
-	while (1 == (ret = io_getevents(ctx, 0, 1, &event, &timeout))) {
-		io_callback_t cb = (io_callback_t)event.data;
-		struct iocb *iocb = event.obj;
+	struct io_event events[IO_BATCH_EVENTS];
+	struct io_event *ep;
+	int ret = 0;		/* total number of events processed */
+	int n;
+
+	/*
+	 * Process io events and call the callbacks.
+	 * Try to batch the events up to IO_BATCH_EVENTS at a time.
+	 * Loop until we have read all the available events and called the callbacks.
+	 */
+	do {
+		int i;
+
+		if ((n = io_getevents(ctx, 1, IO_BATCH_EVENTS, &events, timeout)) <= 0)
+			break;
+		ret += n;
+		for (ep = events, i = n; i-- > 0; ep++) {
+			io_callback_t cb = (io_callback_t)ep->data;
+			struct iocb *iocb = ep->obj;
 
-		cb(ctx, iocb, event.res, event.res2);
-	}
+			cb(ctx, iocb, ep->res, ep->res2);
+		}
+	} while (n == IO_BATCH_EVENTS);
 
-	return ret;
+	return ret ? ret : n;		/* return number of events or error */
 }
diff -rupN -X /home/daniel_nfs/dontdiff libaio-0.3.93/src/libaio.h libaio-0.3.93-io_queue_run_with_timeout/src/libaio.h
--- libaio-0.3.93/src/libaio.h	2002-10-08 16:33:42.000000000 -0700
+++ libaio-0.3.93-io_queue_run_with_timeout/src/libaio.h	2003-07-08 16:41:06.471880979 -0700
@@ -123,7 +123,7 @@ extern int io_queue_init(int maxevents, 
 /*extern int io_queue_grow(io_context_t ctx, int new_maxevents);*/
 extern int io_queue_release(io_context_t ctx);
 /*extern int io_queue_wait(io_context_t ctx, struct timespec *timeout);*/
-extern int io_queue_run(io_context_t ctx);
+extern int io_queue_run(io_context_t ctx, struct timespec *timeout);
 
 /* Actual syscalls */
 extern int io_setup(int maxevents, io_context_t *ctxp);
Binary files libaio-0.3.93/src/libaio.so.1 and libaio-0.3.93-io_queue_run_with_timeout/src/libaio.so.1 differ
diff -rupN -X /home/daniel_nfs/dontdiff libaio-0.3.93/src/Makefile libaio-0.3.93-io_queue_run_with_timeout/src/Makefile
--- libaio-0.3.93/src/Makefile	2002-09-12 20:30:12.000000000 -0700
+++ libaio-0.3.93-io_queue_run_with_timeout/src/Makefile	2003-07-08 16:47:29.426559122 -0700
@@ -14,7 +14,7 @@ all: $(all_targets)
 
 # libaio provided functions
 libaio_srcs := io_queue_init.c io_queue_release.c
-libaio_srcs += io_queue_wait.c io_queue_run.c
+libaio_srcs += io_queue_run.c
 
 # real syscalls
 libaio_srcs += io_getevents.c io_submit.c io_cancel.c

[-- Attachment #3: patch.libaio.man.io_queue_run --]
[-- Type: text/x-patch, Size: 1462 bytes --]

diff -rupN -X /home/daniel_nfs/dontdiff libaio-0.3.93/man/io_queue_run.3 libaio-0.3.93-io_queue_run_with_timeout/man/io_queue_run.3
--- libaio-0.3.93/man/io_queue_run.3	2002-09-26 08:55:18.000000000 -0700
+++ libaio-0.3.93-io_queue_run_with_timeout/man/io_queue_run.3	2003-07-08 17:28:55.441862077 -0700
@@ -9,16 +9,21 @@ io_queue_run \- Handle completed io requ
 .B #include <libaio.h>
 .br
 .sp
-.BI "int io_queue_run(io_context_t  ctx );"
+.BI "int io_queue_run(io_context_t  ctx, struct timespec *timeout);"
 .sp
 .fi
 .SH DESCRIPTION
 .B io_queue_run
-Attempts to read  all the events events from
-the completion queue for the aio_context specified by ctx_id.
+Attempts to read all the events events from
+the completion queue for the aio_context specified by ctx and calls the callbacks
+setup by io_set_callback().
 .SH "RETURN VALUES"
+Returns the number of events handled.
 May return
-0 if no events are available.
+0 if no events are available and the timeout specified
+by timeout has elapsed, where timeout == NULL specifies an infinite
+timeout.  Note that the timeout pointed to by timeout is relative and
+will be updated if not NULL and the operation blocks.
 Will fail with -ENOSYS if not implemented.
 .SH ERRORS
 .TP
@@ -44,7 +49,6 @@ Not implemented
 .BR io_prep_pwrite(3),
 .BR io_queue_init(3),
 .BR io_queue_release(3),
-.BR io_queue_wait(3),
 .BR io_set_callback(3),
 .BR io_submit(3),
 .BR errno(3)

Re: [PATCH libaio] add timeout to io_queue_run and remove io_queue_wait

[Daniel McNeil]

On Wed, 2003-07-09 at 15:31, John Myers wrote:
> Daniel McNeil wrote:
> 
> >Thoughts?
> >  
> >
> I don't think io_queue_run() is particularly worthwhile.  Applications 
> are much better off coding that loop themselves, so they can control 
> such things as how many events they ask for in a call to io_getevents() 
> and how they handle process shutdown.  Besides, io_queue_run()'s 
> handling of EINTR isn't particularly good.
> 
> io_queue_run() is basically for legacy apps.  Your patch changes its 
> signature, which breaks its only use.
> 

If any application is using io_queue_run()/io_queue_wait(), it is
wasting a bunch of cpu time because io_queue_wait() is never waiting.
So, yes, this would break any existing application which is using
the currently broken io_queue_wait().  Is there any "legacy" app using
this?  How long have the interfaces been around?

If io_queue_run()/io_queue_wait() isn't worthwhile, then it should be
removed.  If having a callback interface is worthwhile, I vote for
fixing it.  So the choices are:

1. patch kernel to get io_queue_wait() to actually wait. (see my earlier
   patch).

2. Change the libaio callback interfaces so that it blocks without
   patching the kernel.  (This patch does that).

3. remove io_queue_run()/io_queue_wait() if they are not worthwhile.

I agree that the io_getevents() is sufficient, but it doesn't hurt
having the callback interface (if it actually worked correctly).

Daniel

Re: [PATCH libaio] add timeout to io_queue_run and remove io_queue_wait

[John Myers]

Looking through the linux-aio archives, it seems I  was confusing 
io_queue_run() with io_queue_init() and io_queue_release(), which Ben 
LaHaise reports as being used by Oracle.  In any case, I have no 
objection to removing io_queue_run() or io_queue_wait().  But then I 
don't maintain the library.