libaio: resurrect aio poll and add io_pgetevents support

libaio: resurrect aio poll and add io_pgetevents support

[Christoph Hellwig]

Hi all,

this series resurrects IOCB_CMD_POLL support and adds support for the
new io_pgetevents system call, as well as adding a test case.

Git branch:

    git://git.infradead.org/users/hch/libaio.git aio-poll

Gitweb:

    http://git.infradead.org/users/hch/libaio.git/shortlog/refs/heads/aio-poll

[PATCH 1/6] resurrect aio poll support

[Christoph Hellwig]

Now that we have poll support in mainline, remove comments about the
do not use status.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 src/libaio.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/libaio.h b/src/libaio.h
index a5cd2e1..a1a8fc4 100644
--- a/src/libaio.h
+++ b/src/libaio.h
@@ -43,7 +43,7 @@ typedef enum io_iocb_cmd {
 	IO_CMD_FSYNC = 2,
 	IO_CMD_FDSYNC = 3,
 
-	IO_CMD_POLL = 5, /* Never implemented in mainline, see io_prep_poll */
+	IO_CMD_POLL = 5,
 	IO_CMD_NOOP = 6,
 	IO_CMD_PREADV = 7,
 	IO_CMD_PWRITEV = 8,
@@ -236,8 +236,6 @@ static inline void io_prep_pwritev2(struct iocb *iocb, int fd, const struct iove
 	iocb->u.c.offset = offset;
 }
 
-/* Jeff Moyer says this was implemented in Red Hat AS2.1 and RHEL3.
- * AFAICT, it was never in mainline, and should not be used. --RR */
 static inline void io_prep_poll(struct iocb *iocb, int fd, int events)
 {
         memset(iocb, 0, sizeof(*iocb));
-- 
2.14.2

[PATCH 2/6] move _body_io_syscall to the generic syscall.h

[Christoph Hellwig]

This way it can be used for the fallback 6-argument version on
all architectures.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 src/syscall-generic.h | 6 ------
 src/syscall.h         | 7 +++++++
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/syscall-generic.h b/src/syscall-generic.h
index 24d7c7c..35b8580 100644
--- a/src/syscall-generic.h
+++ b/src/syscall-generic.h
@@ -2,12 +2,6 @@
 #include <unistd.h>
 #include <sys/syscall.h>
 
-#define _body_io_syscall(sname, args...) \
-{ \
-	int ret = syscall(__NR_##sname, ## args); \
-	return ret < 0 ? -errno : ret; \
-}
-
 #define io_syscall1(type,fname,sname,type1,arg1) \
 type fname(type1 arg1) \
 _body_io_syscall(sname, (long)arg1)
diff --git a/src/syscall.h b/src/syscall.h
index a2da030..3819519 100644
--- a/src/syscall.h
+++ b/src/syscall.h
@@ -10,6 +10,13 @@
 #define DEFSYMVER(compat_sym, orig_sym, ver_sym)	\
 	__asm__(".symver " SYMSTR(compat_sym) "," SYMSTR(orig_sym) "@@LIBAIO_" SYMSTR(ver_sym));
 
+/* generic fallback */
+#define _body_io_syscall(sname, args...) \
+{ \
+	int ret = syscall(__NR_##sname, ## args); \
+	return ret < 0 ? -errno : ret; \
+}
+
 #if defined(__i386__)
 #include "syscall-i386.h"
 #elif defined(__x86_64__)
-- 
2.14.2

[PATCH 3/6] provide a generic io_syscall6

[Christoph Hellwig]

This allows to call a 6-argument syscall using the generic syscall()
function from libc.  No arch-specific version is specified as it would
be a lot of effort for very little gain.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 src/syscall.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/syscall.h b/src/syscall.h
index 3819519..e7ff81b 100644
--- a/src/syscall.h
+++ b/src/syscall.h
@@ -39,3 +39,12 @@
 #warning "using generic syscall method"
 #include "syscall-generic.h"
 #endif
+
+#ifndef io_syscall6
+#define io_syscall6(type,fname,sname,type1,arg1,type2,arg2,type3,arg3, \
+		type4,arg4,type5,arg5,type6,arg6) \
+type fname (type1 arg1,type2 arg2,type3 arg3,type4 arg4,type5 arg5, \
+		type6 arg6) \
+_body_io_syscall(sname, (long)arg1, (long)arg2, (long)arg3, (long)arg4, \
+               (long)arg5, (long)arg5)
+#endif /* io_syscall6 */
-- 
2.14.2

[PATCH 4/6] move the aio_ring defintion and empty check into a header

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 src/aio_ring.h     | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 src/io_getevents.c | 28 +++-------------------------
 2 files changed, 52 insertions(+), 25 deletions(-)
 create mode 100644 src/aio_ring.h

diff --git a/src/aio_ring.h b/src/aio_ring.h
new file mode 100644
index 0000000..3842c4b
--- /dev/null
+++ b/src/aio_ring.h
@@ -0,0 +1,49 @@
+/*
+   libaio Linux async I/O interface
+   Copyright 2002 Red Hat, Inc.
+
+   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 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.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, write to the Free Software
+   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
+ */
+#ifndef _AIO_RING_H
+#define _AIO_RING_H
+
+#define AIO_RING_MAGIC                  0xa10a10a1
+
+struct aio_ring {
+	unsigned        id;     /* kernel internal index number */
+	unsigned        nr;     /* number of io_events */
+	unsigned        head;
+	unsigned        tail;
+
+	unsigned        magic;
+	unsigned        compat_features;
+	unsigned        incompat_features;
+	unsigned        header_length;  /* size of aio_ring */
+};
+
+static inline int aio_ring_is_empty(io_context_t ctx, struct timespec *timeout)
+{
+	struct aio_ring *ring = (struct aio_ring *)ctx;
+
+	if (!ring || ring->magic != AIO_RING_MAGIC)
+		return 0;
+	if (!timeout || timeout->tv_sec || timeout->tv_nsec)
+		return 0;
+	if (ring->head != ring->tail)
+		return 0;
+	return 1;
+}
+
+#endif /* _AIO_RING_H */
diff --git a/src/io_getevents.c b/src/io_getevents.c
index 5a05174..90d6081 100644
--- a/src/io_getevents.c
+++ b/src/io_getevents.c
@@ -21,36 +21,14 @@
 #include <stdlib.h>
 #include <time.h>
 #include "syscall.h"
+#include "aio_ring.h"
 
 io_syscall5(int, __io_getevents_0_4, io_getevents, io_context_t, ctx, long, min_nr, long, nr, struct io_event *, events, struct timespec *, timeout)
 
-#define AIO_RING_MAGIC                  0xa10a10a1
-
-/* Ben will hate me for this */
-struct aio_ring {
-	unsigned        id;     /* kernel internal index number */
-	unsigned        nr;     /* number of io_events */
-	unsigned        head;
-	unsigned        tail;
- 
-	unsigned        magic;
-	unsigned        compat_features;
-	unsigned        incompat_features;
-	unsigned        header_length;  /* size of aio_ring */
-};
-
 int io_getevents_0_4(io_context_t ctx, long min_nr, long nr, struct io_event * events, struct timespec * timeout)
 {
-	struct aio_ring *ring;
-	ring = (struct aio_ring*)ctx;
-	if (ring==NULL || ring->magic != AIO_RING_MAGIC)
-		goto do_syscall;
-	if (timeout!=NULL && timeout->tv_sec == 0 && timeout->tv_nsec == 0) {
-		if (ring->head == ring->tail)
-			return 0;
-	}
-	
-do_syscall:	
+	if (aio_ring_is_empty(ctx, timeout))
+		return 0;
 	return __io_getevents_0_4(ctx, min_nr, nr, events, timeout);
 }
 
-- 
2.14.2

[PATCH 5/6] add support for io_pgetevents

[Christoph Hellwig]

This is ppoll/pselect equivalent for io_getevents.  It atomically executes
the following sequence:

	sigset_t origmask;

	pthread_sigmask(SIG_SETMASK, &sigmask, &origmask);
	ret = io_getevents(ctx, min_nr, nr, events, timeout);
	pthread_sigmask(SIG_SETMASK, &origmask, NULL);

And thus allows to safely mix aio and signals, especially together with
IO_CMD_POLL.  See the pselect(2) man page for a more detailed explanation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 man/io_getevents.3   | 61 +++++++++++++++++++++++++++++++++++++++++++++++++---
 src/Makefile         |  2 +-
 src/io_pgetevents.c  | 47 ++++++++++++++++++++++++++++++++++++++++
 src/libaio.h         |  4 ++++
 src/libaio.map       |  5 +++++
 src/syscall-i386.h   |  1 +
 src/syscall-x86_64.h |  1 +
 7 files changed, 117 insertions(+), 4 deletions(-)
 create mode 100644 src/io_pgetevents.c

diff --git a/man/io_getevents.3 b/man/io_getevents.3
index 8e9ddc8..5062daa 100644
--- a/man/io_getevents.3
+++ b/man/io_getevents.3
@@ -18,7 +18,7 @@
 ./"
 .TH io_getevents 2 2002-09-03 "Linux 2.4" "Linux AIO"
 .SH NAME
-io_getevents \- Read resulting events from io requests
+io_getevents, aio_pgetevents \- Read resulting events from io requests
 .SH SYNOPSIS
 .nf
 .B #include <errno.h>
@@ -43,7 +43,7 @@ struct io_event {
 };
 .sp
 .BI "int io_getevents(io_context_t " ctx ",  long " nr ", struct io_event *" events "[], struct timespec *" timeout ");"
-
+.BI "int io_pgetevents(io_context_t " ctx ",  long " nr ", struct io_event *" events "[], struct timespec *" timeout ", sigset_t *" sigmask ");"
 .fi
 .SH DESCRIPTION
 Attempts to read  up to nr events from
@@ -55,6 +55,60 @@ by when has elapsed, where when == NULL specifies an infinite
 timeout.  Note that the timeout pointed to by when is relative and
 will be updated if not NULL and the operation blocks.  Will fail
 with ENOSYS if not implemented.
+.SS io_pgetevents()
+The relationship between
+.BR io_getevents ()
+and
+.BR io_pgetevents ()
+is analogous to the relationship between
+.BR select (2)
+and
+.BR pselect (2):
+similar
+.BR pselect (2),
+.BR pgetevents ()
+allows an application to safely wait until either an aio completion
+events happens or until a signal is caught.
+.PP
+The following
+.BR io_pgetevents ()
+call:
+call:
+.PP
+.in +4n
+.EX
+ret = io_pgetevents(ctx, min_nr, nr, events, timeout, sigmask);
+.EE
+.in
+.PP
+is equivalent to
+.I atomically
+executing the following calls:
+.PP
+.in +4n
+.EX
+sigset_t origmask;
+
+pthread_sigmask(SIG_SETMASK, &sigmask, &origmask);
+ret = io_getevents(ctx, min_nr, nr, events, timeout);
+pthread_sigmask(SIG_SETMASK, &origmask, NULL);
+.EE
+.in
+.PP
+See the description of
+.BR pselect (2)
+for an explanation of why
+.BR io_pgetevents ()
+is necessary.
+.PP
+If the
+.I sigmask
+argument is specified as NULL, then no signal mask manipulation is
+performed (and thus
+.BR io_pgetevents ()
+behaves the same as
+.BR io_getevents()
+) .
 .SH ERRORS
 .TP
 .B EINVAL 
@@ -76,4 +130,5 @@ if any of the memory specified to is invalid.
 .BR io_queue_wait(3),
 .BR io_set_callback(3),
 .BR io_submit(3),
-.BR errno(3)
+.BR errno(3),
+.BR pselect(2)
diff --git a/src/Makefile b/src/Makefile
index eadb336..f5a57d3 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -23,7 +23,7 @@ libaio_srcs += io_queue_wait.c io_queue_run.c
 
 # real syscalls
 libaio_srcs += io_getevents.c io_submit.c io_cancel.c
-libaio_srcs += io_setup.c io_destroy.c
+libaio_srcs += io_setup.c io_destroy.c io_pgetevents.c
 
 # internal functions
 libaio_srcs += raw_syscall.c
diff --git a/src/io_pgetevents.c b/src/io_pgetevents.c
new file mode 100644
index 0000000..47dbbb7
--- /dev/null
+++ b/src/io_pgetevents.c
@@ -0,0 +1,47 @@
+/*
+   libaio Linux async I/O interface
+   Copyright 2018 Christoph Hellwig.
+
+   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 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.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, write to the Free Software
+   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
+ */
+#include <libaio.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <time.h>
+#include "syscall.h"
+#include "aio_ring.h"
+
+#ifdef __NR_io_pgetevents
+io_syscall6(int, __io_pgetevents, io_pgetevents, io_context_t, ctx, long,
+		min_nr, long, nr, struct io_event *, events,
+		struct timespec *, timeout, sigset_t *, sigmask);
+
+int io_pgetevents(io_context_t ctx, long min_nr, long nr,
+		struct io_event *events, struct timespec *timeout,
+		sigset_t *sigmask)
+{
+	if (aio_ring_is_empty(ctx, timeout))
+		return 0;
+	return __io_pgetevents(ctx, min_nr, nr, events, timeout, sigmask);
+}
+#else
+int io_pgetevents(io_context_t ctx, long min_nr, long nr,
+		struct io_event *events, struct timespec *timeout,
+		sigset_t *sigmask)
+
+{
+	return -ENOSYS;
+}
+#endif /* __NR_io_pgetevents */
diff --git a/src/libaio.h b/src/libaio.h
index a1a8fc4..f356c97 100644
--- a/src/libaio.h
+++ b/src/libaio.h
@@ -29,6 +29,7 @@ extern "C" {
 
 #include <sys/types.h>
 #include <string.h>
+#include <signal.h>
 
 struct timespec;
 struct sockaddr;
@@ -161,6 +162,9 @@ extern int io_destroy(io_context_t ctx);
 extern int io_submit(io_context_t ctx, long nr, struct iocb *ios[]);
 extern int io_cancel(io_context_t ctx, struct iocb *iocb, struct io_event *evt);
 extern int io_getevents(io_context_t ctx_id, long min_nr, long nr, struct io_event *events, struct timespec *timeout);
+extern int io_pgetevents(io_context_t ctx_id, long min_nr, long nr,
+		struct io_event *events, struct timespec *timeout,
+		sigset_t *sigmask);
 
 
 static inline void io_set_callback(struct iocb *iocb, io_callback_t cb)
diff --git a/src/libaio.map b/src/libaio.map
index dc37725..ec9d13b 100644
--- a/src/libaio.map
+++ b/src/libaio.map
@@ -20,3 +20,8 @@ LIBAIO_0.4 {
 		io_getevents;
 		io_queue_wait;
 } LIBAIO_0.1;
+
+LIBAIO_0.5 {
+	global:
+		io_pgetevents;
+} LIBAIO_0.4;
diff --git a/src/syscall-i386.h b/src/syscall-i386.h
index 9576975..e312a3f 100644
--- a/src/syscall-i386.h
+++ b/src/syscall-i386.h
@@ -3,6 +3,7 @@
 #define __NR_io_getevents	247
 #define __NR_io_submit		248
 #define __NR_io_cancel		249
+#define __NR_io_pgetevents	385
 
 #define io_syscall1(type,fname,sname,type1,arg1)	\
 type fname(type1 arg1)					\
diff --git a/src/syscall-x86_64.h b/src/syscall-x86_64.h
index 9361856..f811ce4 100644
--- a/src/syscall-x86_64.h
+++ b/src/syscall-x86_64.h
@@ -3,6 +3,7 @@
 #define __NR_io_getevents	208
 #define __NR_io_submit		209
 #define __NR_io_cancel		210
+#define __NR_io_pgetevents	333
 
 #define __syscall_clobber "r11","rcx","memory" 
 #define __syscall "syscall"
-- 
2.14.2

[PATCH 6/6] add test for aio poll and io_pgetevents

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 harness/cases/22.t | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 149 insertions(+)
 create mode 100644 harness/cases/22.t

diff --git a/harness/cases/22.t b/harness/cases/22.t
new file mode 100644
index 0000000..a9fec6b
--- /dev/null
+++ b/harness/cases/22.t
@@ -0,0 +1,149 @@
+/*
+ * Copyright (C) 2006-2018 Free Software Foundation, Inc.
+ * Copyright (C) 2018 Christoph Hellwig.
+ * License: LGPLv2.1 or later.
+ *
+ * Description: test aio poll and io_pgetevents signal handling.
+ *
+ * Very roughly based on glibc tst-pselect.c.
+ */
+#include <errno.h>
+#include <signal.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/poll.h>
+#include <sys/wait.h>
+#include <stdlib.h>
+
+static volatile int handler_called;
+
+static void
+handler(int sig)
+{
+	handler_called = 1;
+}
+
+int test_main(void)
+{
+	struct timespec to = { .tv_sec = 0, .tv_nsec = 500000000 };
+	pid_t parent = getpid(), p;
+	int pipe1[2], pipe2[2];
+	struct sigaction sa = { .sa_flags = 0 };
+	sigset_t sigmask;
+	struct io_context *ctx = NULL;
+	struct io_event ev;
+	struct iocb iocb;
+	struct iocb *iocbs[] = { &iocb };
+	int ret;
+
+	sigemptyset(&sa.sa_mask);
+
+	sa.sa_handler = handler;
+	if (sigaction(SIGUSR1, &sa, NULL) != 0) {
+		printf("sigaction(1) failed\n");
+		return 1;
+	}
+
+	sa.sa_handler = SIG_IGN;
+	if (sigaction(SIGCHLD, &sa, NULL) != 0) {
+		printf("sigaction(2) failed\n");
+		return 1;
+	}
+
+	sigemptyset(&sigmask);
+	sigaddset(&sigmask, SIGUSR1);
+	if (sigprocmask(SIG_BLOCK, &sigmask, NULL) != 0) {
+		printf("sigprocmask failed\n");
+		return 1;
+	}
+
+	if (pipe(pipe1) != 0 || pipe(pipe2) != 0) {
+		printf("pipe failed\n");
+		return 1;
+	}
+
+	sigprocmask(SIG_SETMASK, NULL, &sigmask);
+	sigdelset(&sigmask, SIGUSR1);
+
+	p = fork();
+	switch (p) {
+	case -1:
+		printf("fork failed\n");
+		exit(2);
+	case 0:
+		close(pipe1[1]);
+		close(pipe2[0]);
+
+		ret = io_setup(1, &ctx);
+		if (ret) {
+			printf("child: io_setup failed\n");
+			return 1;
+		}
+
+		io_prep_poll(&iocb, pipe1[0], POLLIN);
+		ret = io_submit(ctx, 1, iocbs);
+		if (ret != 1) {
+			printf("child: io_submit failed\n");
+			return 1;
+		}
+
+		do {
+			if (getppid() != parent) {
+				printf("parent died\n");
+				exit(2);
+			}
+			ret = io_pgetevents(ctx, 1, 1, &ev, &to, &sigmask);
+		} while (ret == 0);
+
+		if (ret != -EINTR) {
+			printf("child: io_pgetevents did not set errno to EINTR\n");
+			return 1;
+		}
+
+		do {
+			errno = 0;
+			ret = write(pipe2[1], "foo", 3);
+		} while (ret == -1 && errno == EINTR);
+
+		exit(0);
+	default:
+		close(pipe1[0]);
+		close(pipe2[1]);
+
+		io_prep_poll(&iocb, pipe2[0], POLLIN);
+
+		ret = io_setup(1, &ctx);
+		if (ret) {
+			printf("child: io_setup failed\n");
+			return 1;
+		}
+
+		ret = io_submit(ctx, 1, iocbs);
+		if (ret != 1) {
+			printf("child: io_submit failed\n");
+			return 1;
+		}
+
+		kill(p, SIGUSR1);
+
+		ret = io_pgetevents(ctx, 1, 1, &ev, NULL, &sigmask);
+		if (ret < 0) {
+			printf("parent: io_pgetevents failed\n");
+			return 1;
+		}
+		if (ret != 1) {
+			printf("parent: io_pgetevents did not report event\n");
+			return 1;
+		}
+		if (ev.obj != &iocb) {
+			printf("parent: io_pgetevents reports wrong fd\n");
+			return 1;
+		}
+		if (ev.res != POLLIN) {
+			printf("parent: io_pgetevents did not report readable fd\n");
+			return 1;
+		}
+
+		return 0;
+	}
+}
-- 
2.14.2

Re: [PATCH 6/6] add test for aio poll and io_pgetevents

[Philippe Ombredanne]

Dear Christoph,

On Thu, Jan 4, 2018 at 9:03 AM, Christoph Hellwig <hch@lst.de> wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  harness/cases/22.t | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 149 insertions(+)
>  create mode 100644 harness/cases/22.t
>
> diff --git a/harness/cases/22.t b/harness/cases/22.t
> new file mode 100644
> index 0000000..a9fec6b
> --- /dev/null
> +++ b/harness/cases/22.t
> @@ -0,0 +1,149 @@
> +/*
> + * Copyright (C) 2006-2018 Free Software Foundation, Inc.
> + * Copyright (C) 2018 Christoph Hellwig.
> + * License: LGPLv2.1 or later.

Would you consider using an SPDX tag instead as documented in Thomas
doc patches [1]? This rather close to what you use today and would
come out as this, on the first line:

SPDX-License-Identifier: LGPL-2.1+

Thank you!

[1] https://lkml.org/lkml/2017/12/28/323
-- 
Cordially
Philippe Ombredanne, your kernel licensing scruffy

Re: [PATCH 6/6] add test for aio poll and io_pgetevents

[Christoph Hellwig]

On Thu, Jan 04, 2018 at 11:24:16AM +0100, Philippe Ombredanne wrote:
> Would you consider using an SPDX tag instead as documented in Thomas
> doc patches [1]? This rather close to what you use today and would
> come out as this, on the first line:
> 
> SPDX-License-Identifier: LGPL-2.1+
> 
> Thank you!

Only if the libaio maintainers do the work of actually explaning what
such tags mean in their project, because without that it would be
entirely meaningless.

Re: [PATCH 2/6] move _body_io_syscall to the generic syscall.h

[Jeff Moyer]

Christoph Hellwig <hch@lst.de> writes:

> This way it can be used for the fallback 6-argument version on
> all architectures.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

This is a strange way to do things.  However, I was never really sold on
libaio having to implement its own system call wrappers.  That decision
definitely resulted in some maintenance overhead.

Ben, what was your reasoning for not just using syscall?

-Jeff

> ---
>  src/syscall-generic.h | 6 ------
>  src/syscall.h         | 7 +++++++
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/src/syscall-generic.h b/src/syscall-generic.h
> index 24d7c7c..35b8580 100644
> --- a/src/syscall-generic.h
> +++ b/src/syscall-generic.h
> @@ -2,12 +2,6 @@
>  #include <unistd.h>
>  #include <sys/syscall.h>
>  
> -#define _body_io_syscall(sname, args...) \
> -{ \
> -	int ret = syscall(__NR_##sname, ## args); \
> -	return ret < 0 ? -errno : ret; \
> -}
> -
>  #define io_syscall1(type,fname,sname,type1,arg1) \
>  type fname(type1 arg1) \
>  _body_io_syscall(sname, (long)arg1)
> diff --git a/src/syscall.h b/src/syscall.h
> index a2da030..3819519 100644
> --- a/src/syscall.h
> +++ b/src/syscall.h
> @@ -10,6 +10,13 @@
>  #define DEFSYMVER(compat_sym, orig_sym, ver_sym)	\
>  	__asm__(".symver " SYMSTR(compat_sym) "," SYMSTR(orig_sym) "@@LIBAIO_" SYMSTR(ver_sym));
>  
> +/* generic fallback */
> +#define _body_io_syscall(sname, args...) \
> +{ \
> +	int ret = syscall(__NR_##sname, ## args); \
> +	return ret < 0 ? -errno : ret; \
> +}
> +
>  #if defined(__i386__)
>  #include "syscall-i386.h"
>  #elif defined(__x86_64__)

Re: [PATCH 2/6] move _body_io_syscall to the generic syscall.h

[Benjamin LaHaise]

On Fri, Jan 05, 2018 at 11:25:17AM -0500, Jeff Moyer wrote:
> Christoph Hellwig <hch@lst.de> writes:
> 
> > This way it can be used for the fallback 6-argument version on
> > all architectures.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> This is a strange way to do things.  However, I was never really sold on
> libaio having to implement its own system call wrappers.  That decision
> definitely resulted in some maintenance overhead.
> 
> Ben, what was your reasoning for not just using syscall?

The main issue was that glibc's pthreads implementation really sucked back
during initial development and there was a use-case for having the io_XXX
functions usable directly from clone()ed threads that didn't have all the
glibc pthread state setup for per-cpu areas to handle per-thread errno.
That made sense back then, but is rather silly today.

Technically, I'm not sure the generic syscall wrapper is safe to use.  The
io_XXX arch wrappers don't modify errno, while it appears the generic one
does.  That said, nobody has ever noticed...

		-ben

> -Jeff
> 
> > ---
> >  src/syscall-generic.h | 6 ------
> >  src/syscall.h         | 7 +++++++
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/syscall-generic.h b/src/syscall-generic.h
> > index 24d7c7c..35b8580 100644
> > --- a/src/syscall-generic.h
> > +++ b/src/syscall-generic.h
> > @@ -2,12 +2,6 @@
> >  #include <unistd.h>
> >  #include <sys/syscall.h>
> >  
> > -#define _body_io_syscall(sname, args...) \
> > -{ \
> > -	int ret = syscall(__NR_##sname, ## args); \
> > -	return ret < 0 ? -errno : ret; \
> > -}
> > -
> >  #define io_syscall1(type,fname,sname,type1,arg1) \
> >  type fname(type1 arg1) \
> >  _body_io_syscall(sname, (long)arg1)
> > diff --git a/src/syscall.h b/src/syscall.h
> > index a2da030..3819519 100644
> > --- a/src/syscall.h
> > +++ b/src/syscall.h
> > @@ -10,6 +10,13 @@
> >  #define DEFSYMVER(compat_sym, orig_sym, ver_sym)	\
> >  	__asm__(".symver " SYMSTR(compat_sym) "," SYMSTR(orig_sym) "@@LIBAIO_" SYMSTR(ver_sym));
> >  
> > +/* generic fallback */
> > +#define _body_io_syscall(sname, args...) \
> > +{ \
> > +	int ret = syscall(__NR_##sname, ## args); \
> > +	return ret < 0 ? -errno : ret; \
> > +}
> > +
> >  #if defined(__i386__)
> >  #include "syscall-i386.h"
> >  #elif defined(__x86_64__)
> 

-- 
"Thought is the essence of where you are now."

Re: [PATCH 2/6] move _body_io_syscall to the generic syscall.h

[Jeff Moyer]

Hi, Ben,

Thanks for the quick reply.

Benjamin LaHaise <ben@communityfibre.ca> writes:

> On Fri, Jan 05, 2018 at 11:25:17AM -0500, Jeff Moyer wrote:
>> Christoph Hellwig <hch@lst.de> writes:
>> 
>> > This way it can be used for the fallback 6-argument version on
>> > all architectures.
>> >
>> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>> 
>> This is a strange way to do things.  However, I was never really sold on
>> libaio having to implement its own system call wrappers.  That decision
>> definitely resulted in some maintenance overhead.
>> 
>> Ben, what was your reasoning for not just using syscall?
>
> The main issue was that glibc's pthreads implementation really sucked back
> during initial development and there was a use-case for having the io_XXX
> functions usable directly from clone()ed threads that didn't have all the
> glibc pthread state setup for per-cpu areas to handle per-thread errno.
> That made sense back then, but is rather silly today.

Thanks for the background info.

> Technically, I'm not sure the generic syscall wrapper is safe to use.  The
> io_XXX arch wrappers don't modify errno, while it appears the generic one
> does.  That said, nobody has ever noticed...

Good point.  Common architectures don't use the generic syscall wrapper,
so I'm not sure we can conclude that it won't break anything.  At the
same time, I'm not sure I want to write and test the io_syscall6
assembly for all of the supported arches.  I could save and restore
errno.  That sounds ugly, but less painful than the other options.

Does anyone have any strong preferences?

-Jeff

Re: [PATCH 6/6] add test for aio poll and io_pgetevents

[Jeff Moyer]

Christoph Hellwig <hch@lst.de> writes:

> +	p = fork();
> +	switch (p) {
[snip]
> +	default:
> +		close(pipe1[0]);
> +		close(pipe2[1]);
> +
> +		io_prep_poll(&iocb, pipe2[0], POLLIN);
> +
> +		ret = io_setup(1, &ctx);
> +		if (ret) {
> +			printf("child: io_setup failed\n");

parent

> +			return 1;
> +		}
> +
> +		ret = io_submit(ctx, 1, iocbs);
> +		if (ret != 1) {
> +			printf("child: io_submit failed\n");

parent

Other than that, looks ok to me.  Thanks for writing a test!
I can fix this up, no need to repost.

-Jeff

Re: libaio: resurrect aio poll and add io_pgetevents support

[Jeff Moyer]

Christoph Hellwig <hch@lst.de> writes:

> Hi all,
>
> this series resurrects IOCB_CMD_POLL support and adds support for the
> new io_pgetevents system call, as well as adding a test case.

This looks good to me.  There may be a couple of changes to the syscall
bits, but I can take care of that.  I'll review the kernel bits more
thoroughly next week.

-Jeff