Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1306754 > unrolled thread

[PATCH 00/13] aio: thread (work queue) based aio and new aio functionality

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/13] aio: thread (work queue) based aio and new aio functionality Benjamin LaHaise <bcrl@kvack.org> - 2016-01-11 23:10 +0100
    [PATCH 09/13] aio: add support for async openat() Benjamin LaHaise <bcrl@kvack.org> - 2016-01-11 23:10 +0100
      Re: [PATCH 09/13] aio: add support for async openat() Linus Torvalds <torvalds@linux-foundation.org> - 2016-01-12 01:30 +0100
        Re: [PATCH 09/13] aio: add support for async openat() Benjamin LaHaise <bcrl@kvack.org> - 2016-01-12 02:20 +0100
        Re: [PATCH 09/13] aio: add support for async openat() Chris Mason <clm@fb.com> - 2016-01-12 02:50 +0100
        Re: [PATCH 09/13] aio: add support for async openat() Ingo Molnar <mingo@kernel.org> - 2016-01-12 11:00 +0100
    [PATCH 04/13] signals: add and use aio_get_task() to direct signals sent via io_send_sig() Benjamin LaHaise <bcrl@kvack.org> - 2016-01-11 23:10 +0100
    [PATCH 10/13] aio: add async unlinkat functionality Benjamin LaHaise <bcrl@kvack.org> - 2016-01-11 23:10 +0100
    [PATCH 07/13] aio: enabled thread based async fsync Benjamin LaHaise <bcrl@kvack.org> - 2016-01-11 23:10 +0100
      Re: [PATCH 07/13] aio: enabled thread based async fsync Dave Chinner <david@fromorbit.com> - 2016-01-12 02:20 +0100
        Re: [PATCH 07/13] aio: enabled thread based async fsync Linus Torvalds <torvalds@linux-foundation.org> - 2016-01-12 02:30 +0100
          Re: [PATCH 07/13] aio: enabled thread based async fsync Dave Chinner <david@fromorbit.com> - 2016-01-12 03:30 +0100
            Re: [PATCH 07/13] aio: enabled thread based async fsync Linus Torvalds <torvalds@linux-foundation.org> - 2016-01-12 03:40 +0100
              Re: [PATCH 07/13] aio: enabled thread based async fsync Dave Chinner <david@fromorbit.com> - 2016-01-12 04:40 +0100
                Re: [PATCH 07/13] aio: enabled thread based async fsync Linus Torvalds <torvalds@linux-foundation.org> - 2016-01-12 05:10 +0100
                  Re: [PATCH 07/13] aio: enabled thread based async fsync Linus Torvalds <torvalds@linux-foundation.org> - 2016-01-12 05:50 +0100
                    Re: [PATCH 07/13] aio: enabled thread based async fsync Benjamin LaHaise <bcrl@kvack.org> - 2016-01-13 00:00 +0100
                  Re: [PATCH 07/13] aio: enabled thread based async fsync Andy Lutomirski <luto@amacapital.net> - 2016-01-13 00:00 +0100
          Re: [PATCH 07/13] aio: enabled thread based async fsync Paolo Bonzini <pbonzini@redhat.com> - 2016-01-14 10:30 +0100
        Re: [PATCH 07/13] aio: enabled thread based async fsync Benjamin LaHaise <bcrl@kvack.org> - 2016-01-12 02:40 +0100
    [PATCH 06/13] aio: add queue_work() based threaded aio support Benjamin LaHaise <bcrl@kvack.org> - 2016-01-11 23:20 +0100

Page 1 of 2  [1] 2  Next page →

#1306754 — [PATCH 00/13] aio: thread (work queue) based aio and new aio functionality

Hello all,

First off, sorry for the wide reaching To and Cc, but this patch series
touches the core kernel and also reaches across subsystems a bit.  If
some of the people who read this can provide review feedback, I would
very much appreciate it.

This series introduces new AIO functionality to make use of kernel
threads (by way of queue_work()) to implement additional asynchronous
operations.  The work came about as the result of various tuning done to
the kernel for my employer (Solace Systems) that we ship in our
products.

First off, the benefits: using kernel threads to implement AIO
functionality has a significant benefit in our application.  Compared to
a user space thread pool based AIO implementation, we see roughly a 25%
performance improvement in our application by using this new kernel
based AIO functionality.  This comes about as a consequence of fewer
context switches, fewer transitions to/from userspace, and the ability
to make certain optimizations in the kernel that are otherwise
impossible in userspace (ie the new readahead functionality).

Now the downsides: when using queue_work(), code executes in the context
of a different task that the submitter of the operation.  This means
that there are significant security concerns if there are any bugs in
the code that sets up the appropriate security credentials and related
context in struct task.  There may well be DoS bugs in this
implementation which have yet to be discovered.

Given the benefits, I am of the opinion that this patch series is a
useful addition to the kernel.  Since this code will be experimental for
some period of time as the interactions with other subsystems are
reviewed and tested, I have implemented a config option to allow for
this code to be compiled out and a sysctl (fs.aio-auto-threads) that
must be explicitly set to 1 before this new functionality is available
to userspace.  Hopefully this is enough to address the security concerns
during the growing pains and allow other developers to safely explore
the new functionality.

Caveats: the existing O_DIRECT AIO code path is currently bypassed when
the new thread helpers are enabled.  I plan to do additional work in
this area, but the fact that the dio code can block under certain
conditions is not acceptable to the applications I am working on, as it
leads to starvation of other requests the system is processing.  That
said, this is what's ready today, and I hope that people can provide
feedback to help drive further improvements.

I will be posting further documentation and test cases later this week
for people to experiment with, but for those looking for a few test
programs to exercise the new functionality, there is a collection of
code at git://git.kvack.org/aio-testprogs.git/ .  Getting the code
cleaned up from the internal implementation to something that is in
reasonable condition for submission ended up taking longer than
expected.  Thankfully, this kernel cycle lines up with some internal QA
work, so there should be additional testing taking place over the next
couple of months.

Also, the libaio test harness has some bugs that the new functionality
revealed.  A version with fixes for those tests can be fetched from
git://git.kvack.org/~bcrl/libaio.git/ .  Wrappers for the new IOCB_CMD
types should be posted there by the end of the day.

Some notes on the new functionality: all operations are cancellable
providing the kernel subsystem involved aborts operations when delivered
a SIGKILL.  This ensures that async operations on pipe and sockets are
cancelled when the process that issued the operations exits.  A couple
of the test programs exercise this functionality on pipes.

Signal handling is slightly impacted by this AIO functionality.
Specifically, the first patch in the series introduces a new helper,
io_send_sig() that delivers a signal intended for the performer of an io
operation.  This is used to deliver signals like SIGXFS and SIGPIPE.  It
is a straightforward replacement of send_sig(SIGXXX, current, 0) to
io_send_sig(SIGXXX). 

As always, comments, bug reports and feedback are appreciated.
Developers looking for a git pull can find one at
git://git.kvack.org/aio-next.git/ .  Cheers!

		-ben

Benjamin LaHaise (13):
  signals: distinguish signals sent due to i/o via io_send_sig()
  aio: add aio_get_mm() helper
  aio: for async operations, make the iter argument persistent
  signals: add and use aio_get_task() to direct signals sent via
    io_send_sig()
  fs: make do_loop_readv_writev() non-static
  aio: add queue_work() based threaded aio support
  aio: enabled thread based async fsync
  aio: add support for aio poll via aio thread helper
  aio: add support for async openat()
  aio: add async unlinkat functionality
  mm: enable __do_page_cache_readahead() to include present pages
  aio: add support for aio readahead
  aio: add support for aio renameat operation

 drivers/gpu/drm/drm_lock.c     |   2 +-
 drivers/gpu/drm/ttm/ttm_lock.c |   6 +-
 fs/aio.c                       | 727 ++++++++++++++++++++++++++++++++++++++---
 fs/attr.c                      |   2 +-
 fs/binfmt_flat.c               |   2 +-
 fs/fuse/dev.c                  |   2 +-
 fs/internal.h                  |   6 +
 fs/namei.c                     |   2 +-
 fs/pipe.c                      |   4 +-
 fs/read_write.c                |   5 +-
 fs/splice.c                    |   8 +-
 include/linux/aio.h            |   9 +
 include/linux/fs.h             |   3 +
 include/linux/sched.h          |   6 +
 include/uapi/linux/aio_abi.h   |  15 +-
 init/Kconfig                   |  13 +
 kernel/auditsc.c               |   6 +-
 kernel/signal.c                |  20 ++
 kernel/sysctl.c                |   9 +
 mm/filemap.c                   |   6 +-
 mm/internal.h                  |   4 +-
 mm/readahead.c                 |  13 +-
 net/atm/common.c               |   4 +-
 net/ax25/af_ax25.c             |   2 +-
 net/caif/caif_socket.c         |   2 +-
 net/core/stream.c              |   2 +-
 net/decnet/af_decnet.c         |   2 +-
 net/irda/af_irda.c             |   4 +-
 net/netrom/af_netrom.c         |   2 +-
 net/rose/af_rose.c             |   2 +-
 net/sctp/socket.c              |   2 +-
 net/unix/af_unix.c             |   4 +-
 net/x25/af_x25.c               |   2 +-
 33 files changed, 817 insertions(+), 81 deletions(-)

-- 
2.5.0


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

[toc] | [next] | [standalone]

#1306756 — [PATCH 09/13] aio: add support for async openat()

Another blocking operation used by applications that want aio
functionality is that of opening files that are not resident in memory.
Using the thread based aio helper, add support for IOCB_CMD_OPENAT.

Signed-off-by: Benjamin LaHaise <ben.lahaise@solacesystems.com>
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
 fs/aio.c                     | 120 +++++++++++++++++++++++++++++++++++++------
 include/uapi/linux/aio_abi.h |   2 +
 2 files changed, 107 insertions(+), 15 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 4384df4..346786b 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -40,6 +40,8 @@
 #include <linux/ramfs.h>
 #include <linux/percpu-refcount.h>
 #include <linux/mount.h>
+#include <linux/fdtable.h>
+#include <linux/fs_struct.h>
 
 #include <asm/kmap_types.h>
 #include <asm/uaccess.h>
@@ -204,6 +206,9 @@ struct aio_kiocb {
 	unsigned long		ki_rlimit_fsize;
 	aio_thread_work_fn_t	ki_work_fn;
 	struct work_struct	ki_work;
+	struct fs_struct	*ki_fs;
+	struct files_struct	*ki_files;
+	const struct cred	*ki_cred;
 #endif
 };
 
@@ -227,6 +232,7 @@ static const struct address_space_operations aio_ctx_aops;
 static void aio_complete(struct kiocb *kiocb, long res, long res2);
 ssize_t aio_fsync(struct kiocb *iocb, int datasync);
 long aio_poll(struct aio_kiocb *iocb);
+long aio_openat(struct aio_kiocb *req);
 
 static __always_inline bool aio_may_use_threads(void)
 {
@@ -1496,6 +1502,9 @@ static int aio_thread_queue_iocb_cancel(struct kiocb *kiocb)
 static void aio_thread_fn(struct work_struct *work)
 {
 	struct aio_kiocb *iocb = container_of(work, struct aio_kiocb, ki_work);
+	struct files_struct *old_files = current->files;
+	const struct cred *old_cred = current_cred();
+	struct fs_struct *old_fs = current->fs;
 	kiocb_cancel_fn *old_cancel;
 	long ret;
 
@@ -1503,6 +1512,13 @@ static void aio_thread_fn(struct work_struct *work)
 	current->kiocb = &iocb->common;		/* For io_send_sig(). */
 	WARN_ON(atomic_read(&current->signal->sigcnt) != 1);
 
+	if (iocb->ki_fs)
+		current->fs = iocb->ki_fs;
+	if (iocb->ki_files)
+		current->files = iocb->ki_files;
+	if (iocb->ki_cred)
+		current->cred = iocb->ki_cred;
+
 	/* Check for early stage cancellation and switch to late stage
 	 * cancellation if it has not already occurred.
 	 */
@@ -1519,6 +1535,19 @@ static void aio_thread_fn(struct work_struct *work)
 		     ret == -ERESTARTNOHAND || ret == -ERESTART_RESTARTBLOCK))
 		ret = -EINTR;
 
+	if (iocb->ki_cred) {
+		current->cred = old_cred;
+		put_cred(iocb->ki_cred);
+	}
+	if (iocb->ki_files) {
+		current->files = old_files;
+		put_files_struct(iocb->ki_files);
+	}
+	if (iocb->ki_fs) {
+		exit_fs(current);
+		current->fs = old_fs;
+	}
+
 	/* Completion serializes cancellation by taking ctx_lock, so
 	 * aio_complete() will not return until after force_sig() in
 	 * aio_thread_queue_iocb_cancel().  This should ensure that
@@ -1530,6 +1559,9 @@ static void aio_thread_fn(struct work_struct *work)
 }
 
 #define AIO_THREAD_NEED_TASK	0x0001	/* Need aio_kiocb->ki_submit_task */
+#define AIO_THREAD_NEED_FS	0x0002	/* Need aio_kiocb->ki_fs */
+#define AIO_THREAD_NEED_FILES	0x0004	/* Need aio_kiocb->ki_files */
+#define AIO_THREAD_NEED_CRED	0x0008	/* Need aio_kiocb->ki_cred */
 
 /* aio_thread_queue_iocb
  *	Queues an aio_kiocb for dispatch to a worker thread.  Prepares the
@@ -1547,6 +1579,20 @@ static ssize_t aio_thread_queue_iocb(struct aio_kiocb *iocb,
 		iocb->ki_submit_task = current;
 		get_task_struct(iocb->ki_submit_task);
 	}
+	if (flags & AIO_THREAD_NEED_FS) {
+		struct fs_struct *fs = current->fs;
+
+		iocb->ki_fs = fs;
+		spin_lock(&fs->lock);
+		fs->users++;
+		spin_unlock(&fs->lock);
+	}
+	if (flags & AIO_THREAD_NEED_FILES) {
+		iocb->ki_files = current->files;
+		atomic_inc(&iocb->ki_files->count);
+	}
+	if (flags & AIO_THREAD_NEED_CRED)
+		iocb->ki_cred = get_current_cred();
 
 	/* Cancellation needs to be always available for operations performed
 	 * using helper threads.  Prior to the iocb being assigned to a worker
@@ -1716,22 +1762,54 @@ long aio_poll(struct aio_kiocb *req)
 {
 	return aio_thread_queue_iocb(req, aio_thread_op_poll, 0);
 }
+
+static long aio_thread_op_openat(struct aio_kiocb *req)
+{
+	u64 buf, offset;
+	long ret;
+	u32 fd;
+
+	use_mm(req->ki_ctx->mm);
+	if (unlikely(__get_user(fd, &req->ki_user_iocb->aio_fildes)))
+		ret = -EFAULT;
+	else if (unlikely(__get_user(buf, &req->ki_user_iocb->aio_buf)))
+		ret = -EFAULT;
+	else if (unlikely(__get_user(offset, &req->ki_user_iocb->aio_offset)))
+		ret = -EFAULT;
+	else {
+		ret = do_sys_open((s32)fd,
+				  (const char __user *)(long)buf,
+				  (int)offset,
+				  (unsigned short)(offset >> 32));
+	}
+	unuse_mm(req->ki_ctx->mm);
+	return ret;
+}
+
+long aio_openat(struct aio_kiocb *req)
+{
+	return aio_thread_queue_iocb(req, aio_thread_op_openat,
+				     AIO_THREAD_NEED_TASK |
+				     AIO_THREAD_NEED_FILES |
+				     AIO_THREAD_NEED_CRED);
+}
 #endif /* IS_ENABLED(CONFIG_AIO_THREAD) */
 
 /*
  * aio_run_iocb:
  *	Performs the initial checks and io submission.
  */
-static ssize_t aio_run_iocb(struct aio_kiocb *req, unsigned opcode,
-			    char __user *buf, size_t len, bool compat)
+static ssize_t aio_run_iocb(struct aio_kiocb *req, struct iocb *user_iocb,
+			    bool compat)
 {
 	struct file *file = req->common.ki_filp;
 	ssize_t ret = -EINVAL;
+	char __user *buf;
 	int rw;
 	fmode_t mode;
 	rw_iter_op *iter_op;
 
-	switch (opcode) {
+	switch (user_iocb->aio_lio_opcode) {
 	case IOCB_CMD_PREAD:
 	case IOCB_CMD_PREADV:
 		mode	= FMODE_READ;
@@ -1768,12 +1846,17 @@ rw_common:
 		if (!iter_op)
 			return -EINVAL;
 
-		if (opcode == IOCB_CMD_PREADV || opcode == IOCB_CMD_PWRITEV)
-			ret = aio_setup_vectored_rw(rw, buf, len,
+		buf = (char __user *)(unsigned long)user_iocb->aio_buf;
+		if (user_iocb->aio_lio_opcode == IOCB_CMD_PREADV ||
+		    user_iocb->aio_lio_opcode == IOCB_CMD_PWRITEV)
+			ret = aio_setup_vectored_rw(rw, buf,
+						    user_iocb->aio_nbytes,
 						    &req->ki_iovec, compat,
 						    &req->ki_iter);
 		else {
-			ret = import_single_range(rw, buf, len, req->ki_iovec,
+			ret = import_single_range(rw, buf,
+						  user_iocb->aio_nbytes,
+						  req->ki_iovec,
 						  &req->ki_iter);
 		}
 		if (!ret)
@@ -1810,6 +1893,11 @@ rw_common:
 			ret = aio_poll(req);
 		break;
 
+	case IOCB_CMD_OPENAT:
+		if (aio_may_use_threads())
+			ret = aio_openat(req);
+		break;
+
 	default:
 		pr_debug("EINVAL: no operation provided\n");
 		return -EINVAL;
@@ -1856,14 +1944,19 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
 	if (unlikely(!req))
 		return -EAGAIN;
 
-	req->common.ki_filp = fget(iocb->aio_fildes);
-	if (unlikely(!req->common.ki_filp)) {
-		ret = -EBADF;
-		goto out_put_req;
+	if (iocb->aio_lio_opcode == IOCB_CMD_OPENAT)
+		req->common.ki_filp = NULL;
+	else {
+		req->common.ki_filp = fget(iocb->aio_fildes);
+		if (unlikely(!req->common.ki_filp)) {
+			ret = -EBADF;
+			goto out_put_req;
+		}
 	}
 	req->common.ki_pos = iocb->aio_offset;
 	req->common.ki_complete = aio_complete;
-	req->common.ki_flags = iocb_flags(req->common.ki_filp);
+	if (req->common.ki_filp)
+		req->common.ki_flags = iocb_flags(req->common.ki_filp);
 
 	if (iocb->aio_flags & IOCB_FLAG_RESFD) {
 		/*
@@ -1891,10 +1984,7 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
 	req->ki_user_iocb = user_iocb;
 	req->ki_user_data = iocb->aio_data;
 
-	ret = aio_run_iocb(req, iocb->aio_lio_opcode,
-			   (char __user *)(unsigned long)iocb->aio_buf,
-			   iocb->aio_nbytes,
-			   compat);
+	ret = aio_run_iocb(req, iocb, compat);
 	if (ret)
 		goto out_put_req;
 
diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
index 7639fb1..0e16988 100644
--- a/include/uapi/linux/aio_abi.h
+++ b/include/uapi/linux/aio_abi.h
@@ -44,6 +44,8 @@ enum {
 	IOCB_CMD_NOOP = 6,
 	IOCB_CMD_PREADV = 7,
 	IOCB_CMD_PWRITEV = 8,
+
+	IOCB_CMD_OPENAT = 9,
 };
 
 /*
-- 
2.5.0


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

[toc] | [prev] | [next] | [standalone]

#1306855 — Re: [PATCH 09/13] aio: add support for async openat()

On Mon, Jan 11, 2016 at 2:07 PM, Benjamin LaHaise <bcrl@kvack.org> wrote:
> Another blocking operation used by applications that want aio
> functionality is that of opening files that are not resident in memory.
> Using the thread based aio helper, add support for IOCB_CMD_OPENAT.

So I think this is ridiculously ugly.

AIO is a horrible ad-hoc design, with the main excuse being "other,
less gifted people, made that design, and we are implementing it for
compatibility because database people - who seldom have any shred of
taste - actually use it".

But AIO was always really really ugly.

Now you introduce the notion of doing almost arbitrary system calls
asynchronously in threads, but then you use that ass-backwards nasty
interface to do so.

Why?

If you want to do arbitrary asynchronous system calls, just *do* it.
But do _that_, not "let's extend this horrible interface in arbitrary
random ways one special system call at a time".

In other words, why is the interface not simply: "do arbitrary system
call X with arguments A, B, C, D asynchronously using a kernel
thread".

That's something that a lot of people might use. In fact, if they can
avoid the nasty AIO interface, maybe they'll even use it for things
like read() and write().

So I really think it would be a nice thing to allow some kind of
arbitrary "queue up asynchronous system call" model.

But I do not think the AIO model should be the model used for that,
even if I think there might be some shared infrastructure.

So I would seriously suggest:

 - how about we add a true "asynchronous system call" interface

 - make it be a list of system calls with a futex completion for each
list entry, so that you can easily wait for the end result that way.

 - maybe (and this is where it gets really iffy) you could even pass
in the result of one system call to the next, so that you can do
things like

       fd = openat(..)
       ret = read(fd, ..)

   asynchronously and then just wait for the read() to complete.

and let us *not* tie this to the aio interface.

In fact, if we do it well, we can go the other way, and try to
implement the nasty AIO interface on top of the generic "just do
things asynchronously".

And I actually think many of your kernel thread parts are good for a
generic implementation. That whole "AIO_THREAD_NEED_CRED" etc logic
all makes sense, although I do suspect you could just make it
unconditional. The cost of a few atomics shouldn't be excessive when
we're talking "use a thread to do op X".

What do you think? Do you think it might be possible to aim for a
generic "do system call asynchronously" model instead?

I'm adding Ingo the to cc, because I think Ingo had a "run this list
of system calls" patch at one point - in order to avoid system call
overhead. I don't think that was very interesting (because system call
overhead is seldom all that noticeable for any interesting system
calls), but with the "let's do the list asynchronously" addition it
might be much more intriguing. Ingo, do I remember correctly that it
was you? I might be confused about who wrote that patch, and I can't
find it now.

               Linus

[toc] | [prev] | [next] | [standalone]

#1306889 — Re: [PATCH 09/13] aio: add support for async openat()

On Mon, Jan 11, 2016 at 04:22:28PM -0800, Linus Torvalds wrote:
> On Mon, Jan 11, 2016 at 2:07 PM, Benjamin LaHaise <bcrl@kvack.org> wrote:
> > Another blocking operation used by applications that want aio
> > functionality is that of opening files that are not resident in memory.
> > Using the thread based aio helper, add support for IOCB_CMD_OPENAT.
> 
> So I think this is ridiculously ugly.
> 
> AIO is a horrible ad-hoc design, with the main excuse being "other,
> less gifted people, made that design, and we are implementing it for
> compatibility because database people - who seldom have any shred of
> taste - actually use it".
> 
> But AIO was always really really ugly.
> 
> Now you introduce the notion of doing almost arbitrary system calls
> asynchronously in threads, but then you use that ass-backwards nasty
> interface to do so.
> 
> Why?

Understood, but there are some reasons behind this.  The core aio submit
mechanism is modeled after the lio_listio() call in posix.  While the
cost of performing syscalls has decreased substantially over the last 10
years, the cost of context switches has not.  Some AIO operations really
want to do part of the work in the context of the original submitter for
the work.  That was/is a critical piece of the async readahead
functionality in this series -- without being able to do a quick return
to the caller when all the cached data is allready resident in the
kernel, there is a significant performance degradation in my tests.  For
other operations which are going to do blocking i/o anyways, the cost of
the context switch often becomes noise.

The async readahead also fills a fills a hole in the proposed extensions
to preadv()/pwritev() -- they need some way to trigger and know when a
readahead operation has completed.  One needs a completion queue of some
sort to figure out which operation has completed in a reasonable
efficient manner.  The futex doesn't really have the ability to do this.

Thread dispatching is another problem the applications I work on
encounter, and AIO helps in this particular area because a thread that
is running hot can simply check the AIO event ring buffer for new events
in its main event loop.  Userspace fundamentally *cannot* do a good job of
dispatching work to threads.  The code I've see other developers come up
with ends up doing things like epoll() in one thread followed by
dispatching the receieved events to different threads.  This ends up
making multiple expensive syscalls (since locking and cross CPU bouncing
is required) when the kernel could just direct things to the right
thread in the first place.

There are a lot of requirements bringing additional complexity that start
to surface once you look at how some of these applications are actually
written.

> If you want to do arbitrary asynchronous system calls, just *do* it.
> But do _that_, not "let's extend this horrible interface in arbitrary
> random ways one special system call at a time".
> 
> In other words, why is the interface not simply: "do arbitrary system
> call X with arguments A, B, C, D asynchronously using a kernel
> thread".

We've had a few proposals to do this, none of which have really managed 
to tackle all the problems that arose.  If we go down this path, we will 
end up needing a table of what syscalls can actually be performed 
asynchronously, and flags indicating what bits of context those syscalls
require.  This does end up looking a bit like how AIO does things
depending on how hard you squint.

I'm not opposed to reworking how AIO dispatches things.  If we're willing 
to relax some constraints (like the hard enforced limits on the number
of AIOs in flight), things can be substantially simplified.  Again,
worries about things like memory usage today are vastly different than
they were back in the early '00s, so the decisions that make sense now
will certainly change the design.

Cancellation is also a concern.  Cancellation is not something that can
be sacrificed.  Without some mechanism to cancel operations that are in
flight, there is no way for a process to cleanly exit.  This patch
series nicely proves that signals work very well for cancellation, and
fit in with a lot of the code we already have.  This implies we would
need to treat threads doing async operations differently from normal
threads.  What happens with the pid namespace?

> That's something that a lot of people might use. In fact, if they can
> avoid the nasty AIO interface, maybe they'll even use it for things
> like read() and write().
> 
> So I really think it would be a nice thing to allow some kind of
> arbitrary "queue up asynchronous system call" model.
> 
> But I do not think the AIO model should be the model used for that,
> even if I think there might be some shared infrastructure.
> 
> So I would seriously suggest:
> 
>  - how about we add a true "asynchronous system call" interface
> 
>  - make it be a list of system calls with a futex completion for each
> list entry, so that you can easily wait for the end result that way.
> 
>  - maybe (and this is where it gets really iffy) you could even pass
> in the result of one system call to the next, so that you can do
> things like
> 
>        fd = openat(..)
>        ret = read(fd, ..)
> 
>    asynchronously and then just wait for the read() to complete.
> 
> and let us *not* tie this to the aio interface.
> 
> In fact, if we do it well, we can go the other way, and try to
> implement the nasty AIO interface on top of the generic "just do
> things asynchronously".
> 
> And I actually think many of your kernel thread parts are good for a
> generic implementation. That whole "AIO_THREAD_NEED_CRED" etc logic
> all makes sense, although I do suspect you could just make it
> unconditional. The cost of a few atomics shouldn't be excessive when
> we're talking "use a thread to do op X".
> 
> What do you think? Do you think it might be possible to aim for a
> generic "do system call asynchronously" model instead?

Maybe it's not too bad to do -- the syscall() primitive is reasonably 
well defined and is supported across architectures, but we're going to 
need new wrappers for *every* syscall supported.  Odds are the work will 
have to be done incrementally to weed out which syscalls are safe and 
which are not, but there is certainly no reason we can't reuse syscall 
numbers and the same argument layout.

Chaining things becomes messy.  There are some cases where that works,
but at least on the applications I've worked on, there tends to be a
fair amount of logic that needs to be run before you can figure out what
and where the next operation is.  The canonical example I can think of
is the case where one is retreiving data from disk.  The first operation
is a read into some table to find out where data is located, the next
operation is a search (binary search in the case I'm thinking of) in the
data that was just read to figure out which record actually contains the
data the app cares about, followed by a read to actually fetch the data
the user actually requires.

And it gets more complicated: different disk i/os need to be issued with
different priorities (something that was not included in what I just
posted today, but is work I plan to propose for merging in the future).
In some cases the priority is known beforehand, but in other cases it
needs to be adjusted dynamically depending on information fetched (users
don't like it if huge i/os completely starve their smaller i/os for
significant amounts of time).

> I'm adding Ingo the to cc, because I think Ingo had a "run this list
> of system calls" patch at one point - in order to avoid system call
> overhead. I don't think that was very interesting (because system call
> overhead is seldom all that noticeable for any interesting system
> calls), but with the "let's do the list asynchronously" addition it
> might be much more intriguing. Ingo, do I remember correctly that it
> was you? I might be confused about who wrote that patch, and I can't
> find it now.

I'd certainly be interested in hearing more ideas concerning
requirements.

Sorry for the giant wall of text...  Nothing is simple! =-)

		-ben

>                Linus

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

[toc] | [prev] | [next] | [standalone]

#1306910 — Re: [PATCH 09/13] aio: add support for async openat()

On Mon, Jan 11, 2016 at 04:22:28PM -0800, Linus Torvalds wrote:
> On Mon, Jan 11, 2016 at 2:07 PM, Benjamin LaHaise <bcrl@kvack.org> wrote:
> > Another blocking operation used by applications that want aio
> > functionality is that of opening files that are not resident in memory.
> > Using the thread based aio helper, add support for IOCB_CMD_OPENAT.
> 
> So I think this is ridiculously ugly.
> 
> AIO is a horrible ad-hoc design, with the main excuse being "other,
> less gifted people, made that design, and we are implementing it for
> compatibility because database people - who seldom have any shred of
> taste - actually use it".
> 
> But AIO was always really really ugly.
> 
> Now you introduce the notion of doing almost arbitrary system calls
> asynchronously in threads, but then you use that ass-backwards nasty
> interface to do so.

[ ... ]

> I'm adding Ingo the to cc, because I think Ingo had a "run this list
> of system calls" patch at one point - in order to avoid system call
> overhead. I don't think that was very interesting (because system call
> overhead is seldom all that noticeable for any interesting system
> calls), but with the "let's do the list asynchronously" addition it
> might be much more intriguing. Ingo, do I remember correctly that it
> was you? I might be confused about who wrote that patch, and I can't
> find it now.

Zach Brown and Ingo traded a bunch of ideas.  There were chicklets and
syslets?  A little search, it looks like acall was a slightly different
iteration, but the patches didn't make it off oss.oracle.com:

https://lwn.net/Articles/316806/

-chris

[toc] | [prev] | [next] | [standalone]

#1307178 — Re: [PATCH 09/13] aio: add support for async openat()

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> What do you think? Do you think it might be possible to aim for a generic "do 
> system call asynchronously" model instead?
> 
> I'm adding Ingo the to cc, because I think Ingo had a "run this list of system 
> calls" patch at one point - in order to avoid system call overhead. I don't 
> think that was very interesting (because system call overhead is seldom all that 
> noticeable for any interesting system calls), but with the "let's do the list 
> asynchronously" addition it might be much more intriguing. Ingo, do I remember 
> correctly that it was you? I might be confused about who wrote that patch, and I 
> can't find it now.

Yeah, it was the whole 'syslets' and 'threadlets' stuff - I had both implemented 
and prototyped into a 'list directory entries asynchronously' testcase.

Threadlets was pretty close to what you are suggesting now. Here's a very good (as 
usual!) writeup from LWN:

  https://lwn.net/Articles/223899/

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1306757 — [PATCH 04/13] signals: add and use aio_get_task() to direct signals sent via io_send_sig()

When a signal is triggered due to an i/o, io_send_sig() needs to deliver
the signal to the task issuing the i/o.  Prepare for thread based aios
by annotating task_struct with a struct kiocb pointer that enables
io_sed_sig() to direct these signals to the submitter of the aio.

Signed-off-by: Benjamin LaHaise <ben.lahaise@solacesystems.com>
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
 fs/aio.c              | 16 ++++++++++++++++
 include/linux/aio.h   |  3 +++
 include/linux/sched.h |  5 +++++
 kernel/signal.c       |  8 +++++++-
 4 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/fs/aio.c b/fs/aio.c
index fc453ca..55c8ff5 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -191,6 +191,9 @@ struct aio_kiocb {
 	struct iov_iter		ki_iter;
 	struct iovec		*ki_iovec;
 	struct iovec		ki_inline_vecs[UIO_FASTIOV];
+
+	/* Fields used for threaded aio helper. */
+	struct task_struct	*ki_submit_task;
 };
 
 /*------ sysctl variables----*/
@@ -586,6 +589,17 @@ struct mm_struct *aio_get_mm(struct kiocb *req)
 	return NULL;
 }
 
+struct task_struct *aio_get_task(struct kiocb *req)
+{
+	if (req->ki_complete == aio_complete) {
+		struct aio_kiocb *iocb;
+
+		iocb = container_of(req, struct aio_kiocb, common);
+		return iocb->ki_submit_task;
+	}
+	return current;
+}
+
 static void free_ioctx(struct work_struct *work)
 {
 	struct kioctx *ctx = container_of(work, struct kioctx, free_work);
@@ -1045,6 +1059,8 @@ static void kiocb_free(struct aio_kiocb *req)
 		eventfd_ctx_put(req->ki_eventfd);
 	if (req->ki_iovec != req->ki_inline_vecs)
 		kfree(req->ki_iovec);
+	if (req->ki_submit_task)
+		put_task_struct(req->ki_submit_task);
 	kmem_cache_free(kiocb_cachep, req);
 }
 
diff --git a/include/linux/aio.h b/include/linux/aio.h
index c5791d4..9a62e8a 100644
--- a/include/linux/aio.h
+++ b/include/linux/aio.h
@@ -18,6 +18,7 @@ extern long do_io_submit(aio_context_t ctx_id, long nr,
 			 struct iocb __user *__user *iocbpp, bool compat);
 void kiocb_set_cancel_fn(struct kiocb *req, kiocb_cancel_fn *cancel);
 struct mm_struct *aio_get_mm(struct kiocb *req);
+struct task_struct *aio_get_task(struct kiocb *req);
 #else
 static inline void exit_aio(struct mm_struct *mm) { }
 static inline long do_io_submit(aio_context_t ctx_id, long nr,
@@ -26,6 +27,8 @@ static inline long do_io_submit(aio_context_t ctx_id, long nr,
 static inline void kiocb_set_cancel_fn(struct kiocb *req,
 				       kiocb_cancel_fn *cancel) { }
 static inline struct mm_struct *aio_get_mm(struct kiocb *req) { return NULL; }
+static inline struct task_struct *aio_get_task(struct kiocb *req)
+{ return current; }
 #endif /* CONFIG_AIO */
 
 /* for sysctl: */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6376d58..bdbf11b 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1633,6 +1633,11 @@ struct task_struct {
 /* journalling filesystem info */
 	void *journal_info;
 
+/* threaded aio info */
+#if IS_ENABLED(CONFIG_AIO)
+	struct kiocb *kiocb;
+#endif
+
 /* stacked block device info */
 	struct bio_list *bio_list;
 
diff --git a/kernel/signal.c b/kernel/signal.c
index 7c14cb4..5da9180 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -34,6 +34,7 @@
 #include <linux/compat.h>
 #include <linux/cn_proc.h>
 #include <linux/compiler.h>
+#include <linux/aio.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/signal.h>
@@ -1432,7 +1433,12 @@ int send_sig_info(int sig, struct siginfo *info, struct task_struct *p)
  */
 int io_send_sig(int sig)
 {
-	return send_sig(sig, current, 0);
+	struct task_struct *task = current;
+#if IS_ENABLED(CONFIG_AIO)
+	if (task->kiocb)
+		task = aio_get_task(task->kiocb);
+#endif
+	return send_sig(sig, task, 0);
 }
 EXPORT_SYMBOL(io_send_sig);
 
-- 
2.5.0


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

[toc] | [prev] | [next] | [standalone]

#1306758 — [PATCH 10/13] aio: add async unlinkat functionality

Enable asynchronous deletion of files by adding support for an aio
unlinkat operation.

Signed-off-by: Benjamin LaHaise <ben.lahaise@solacesystems.com>
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
 fs/aio.c                     | 42 +++++++++++++++++++++++++++++++++---------
 fs/namei.c                   |  2 +-
 include/linux/fs.h           |  1 +
 include/uapi/linux/aio_abi.h |  1 +
 4 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 346786b..3a70492 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -232,7 +232,11 @@ static const struct address_space_operations aio_ctx_aops;
 static void aio_complete(struct kiocb *kiocb, long res, long res2);
 ssize_t aio_fsync(struct kiocb *iocb, int datasync);
 long aio_poll(struct aio_kiocb *iocb);
-long aio_openat(struct aio_kiocb *req);
+
+typedef long (*do_foo_at_t)(int fd, const char *filename, int flags, int mode);
+long aio_do_openat(int fd, const char *filename, int flags, int mode);
+long aio_do_unlinkat(int fd, const char *filename, int flags, int mode);
+long aio_foo_at(struct aio_kiocb *req, do_foo_at_t do_foo_at);
 
 static __always_inline bool aio_may_use_threads(void)
 {
@@ -1763,7 +1767,19 @@ long aio_poll(struct aio_kiocb *req)
 	return aio_thread_queue_iocb(req, aio_thread_op_poll, 0);
 }
 
-static long aio_thread_op_openat(struct aio_kiocb *req)
+long aio_do_openat(int fd, const char *filename, int flags, int mode)
+{
+	return do_sys_open(fd, filename, flags, mode);
+}
+
+long aio_do_unlinkat(int fd, const char *filename, int flags, int mode)
+{
+	if (flags || mode)
+		return -EINVAL;
+	return do_unlinkat(fd, filename);
+}
+
+static long aio_thread_op_foo_at(struct aio_kiocb *req)
 {
 	u64 buf, offset;
 	long ret;
@@ -1777,18 +1793,21 @@ static long aio_thread_op_openat(struct aio_kiocb *req)
 	else if (unlikely(__get_user(offset, &req->ki_user_iocb->aio_offset)))
 		ret = -EFAULT;
 	else {
-		ret = do_sys_open((s32)fd,
-				  (const char __user *)(long)buf,
-				  (int)offset,
-				  (unsigned short)(offset >> 32));
+		do_foo_at_t do_foo_at = (void *)req->ki_data;
+
+		ret = do_foo_at((s32)fd,
+				(const char __user *)(long)buf,
+				(int)offset,
+				(unsigned short)(offset >> 32));
 	}
 	unuse_mm(req->ki_ctx->mm);
 	return ret;
 }
 
-long aio_openat(struct aio_kiocb *req)
+long aio_foo_at(struct aio_kiocb *req, do_foo_at_t do_foo_at)
 {
-	return aio_thread_queue_iocb(req, aio_thread_op_openat,
+	req->ki_data = (unsigned long)(void *)do_foo_at;
+	return aio_thread_queue_iocb(req, aio_thread_op_foo_at,
 				     AIO_THREAD_NEED_TASK |
 				     AIO_THREAD_NEED_FILES |
 				     AIO_THREAD_NEED_CRED);
@@ -1895,7 +1914,12 @@ rw_common:
 
 	case IOCB_CMD_OPENAT:
 		if (aio_may_use_threads())
-			ret = aio_openat(req);
+			ret = aio_foo_at(req, aio_do_openat);
+		break;
+
+	case IOCB_CMD_UNLINKAT:
+		if (aio_may_use_threads())
+			ret = aio_foo_at(req, aio_do_unlinkat);
 		break;
 
 	default:
diff --git a/fs/namei.c b/fs/namei.c
index 0c3974c..84ecc7e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3828,7 +3828,7 @@ EXPORT_SYMBOL(vfs_unlink);
  * writeout happening, and we don't want to prevent access to the directory
  * while waiting on the I/O.
  */
-static long do_unlinkat(int dfd, const char __user *pathname)
+long do_unlinkat(int dfd, const char __user *pathname)
 {
 	int error;
 	struct filename *name;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b3dc406..9051771 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1509,6 +1509,7 @@ extern int vfs_symlink(struct inode *, struct dentry *, const char *);
 extern int vfs_link(struct dentry *, struct inode *, struct dentry *, struct inode **);
 extern int vfs_rmdir(struct inode *, struct dentry *);
 extern int vfs_unlink(struct inode *, struct dentry *, struct inode **);
+extern long do_unlinkat(int dfd, const char __user *pathname);
 extern int vfs_rename(struct inode *, struct dentry *, struct inode *, struct dentry *, struct inode **, unsigned int);
 extern int vfs_whiteout(struct inode *, struct dentry *);
 
diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
index 0e16988..63a0d41 100644
--- a/include/uapi/linux/aio_abi.h
+++ b/include/uapi/linux/aio_abi.h
@@ -46,6 +46,7 @@ enum {
 	IOCB_CMD_PWRITEV = 8,
 
 	IOCB_CMD_OPENAT = 9,
+	IOCB_CMD_UNLINKAT = 10,
 };
 
 /*
-- 
2.5.0


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

[toc] | [prev] | [next] | [standalone]

#1306763 — [PATCH 07/13] aio: enabled thread based async fsync

Enable a fully asynchronous fsync and fdatasync operations in aio using
the aio thread queuing mechanism.

Signed-off-by: Benjamin LaHaise <ben.lahaise@solacesystems.com>
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
 fs/aio.c | 41 +++++++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 88af450..576b780 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -224,8 +224,9 @@ static const struct file_operations aio_ring_fops;
 static const struct address_space_operations aio_ctx_aops;
 
 static void aio_complete(struct kiocb *kiocb, long res, long res2);
+ssize_t aio_fsync(struct kiocb *iocb, int datasync);
 
-static bool aio_may_use_threads(void)
+static __always_inline bool aio_may_use_threads(void)
 {
 #if IS_ENABLED(CONFIG_AIO_THREAD)
 	return !!(aio_auto_threads & 1);
@@ -1654,6 +1655,26 @@ ssize_t generic_async_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 				     AIO_THREAD_NEED_TASK);
 }
 EXPORT_SYMBOL(generic_async_write_iter);
+
+static long aio_thread_op_fsync(struct aio_kiocb *iocb)
+{
+	return vfs_fsync(iocb->common.ki_filp, 0);
+}
+
+static long aio_thread_op_fdatasync(struct aio_kiocb *iocb)
+{
+	return vfs_fsync(iocb->common.ki_filp, 1);
+}
+
+ssize_t aio_fsync(struct kiocb *iocb, int datasync)
+{
+	struct aio_kiocb *req;
+
+	req = container_of(iocb, struct aio_kiocb, common);
+
+	return aio_thread_queue_iocb(req, datasync ? aio_thread_op_fdatasync
+						   : aio_thread_op_fsync, 0);
+}
 #endif /* IS_ENABLED(CONFIG_AIO_THREAD) */
 
 /*
@@ -1664,7 +1685,7 @@ static ssize_t aio_run_iocb(struct aio_kiocb *req, unsigned opcode,
 			    char __user *buf, size_t len, bool compat)
 {
 	struct file *file = req->common.ki_filp;
-	ssize_t ret;
+	ssize_t ret = -EINVAL;
 	int rw;
 	fmode_t mode;
 	rw_iter_op *iter_op;
@@ -1730,17 +1751,17 @@ rw_common:
 		break;
 
 	case IOCB_CMD_FDSYNC:
-		if (!file->f_op->aio_fsync)
-			return -EINVAL;
-
-		ret = file->f_op->aio_fsync(&req->common, 1);
+		if (file->f_op->aio_fsync)
+			ret = file->f_op->aio_fsync(&req->common, 1);
+		else if (file->f_op->fsync && (aio_may_use_threads()))
+			ret = aio_fsync(&req->common, 1);
 		break;
 
 	case IOCB_CMD_FSYNC:
-		if (!file->f_op->aio_fsync)
-			return -EINVAL;
-
-		ret = file->f_op->aio_fsync(&req->common, 0);
+		if (file->f_op->aio_fsync)
+			ret = file->f_op->aio_fsync(&req->common, 0);
+		else if (file->f_op->fsync && (aio_may_use_threads()))
+			ret = aio_fsync(&req->common, 0);
 		break;
 
 	default:
-- 
2.5.0


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

[toc] | [prev] | [next] | [standalone]

#1306888 — Re: [PATCH 07/13] aio: enabled thread based async fsync

On Mon, Jan 11, 2016 at 05:07:23PM -0500, Benjamin LaHaise wrote:
> Enable a fully asynchronous fsync and fdatasync operations in aio using
> the aio thread queuing mechanism.
> 
> Signed-off-by: Benjamin LaHaise <ben.lahaise@solacesystems.com>
> Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>

Insufficient. Needs the range to be passed through and call
vfs_fsync_range(), as I implemented here:

https://lkml.org/lkml/2015/10/28/878

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1306891 — Re: [PATCH 07/13] aio: enabled thread based async fsync

On Mon, Jan 11, 2016 at 5:11 PM, Dave Chinner <david@fromorbit.com> wrote:
>
> Insufficient. Needs the range to be passed through and call
> vfs_fsync_range(), as I implemented here:

And I think that's insufficient *also*.

What you actually want is "sync_file_range()", with the full set of arguments.

Yes, really. Sometimes you want to start the writeback, sometimes you
want to wait for it. Sometimes you want both.

For example, if you are doing your own manual write-behind logic, it
is not sufficient for "wait for data". What you want is "start IO on
new data" followed by "wait for old data to have been written out".

I think this only strengthens my "stop with the idiotic
special-case-AIO magic already" argument.  If we want something more
generic than the usual aio, then we should go all in. Not "let's make
more limited special cases".

              Linus

[toc] | [prev] | [next] | [standalone]

#1306934 — Re: [PATCH 07/13] aio: enabled thread based async fsync

On Mon, Jan 11, 2016 at 05:20:42PM -0800, Linus Torvalds wrote:
> On Mon, Jan 11, 2016 at 5:11 PM, Dave Chinner <david@fromorbit.com> wrote:
> >
> > Insufficient. Needs the range to be passed through and call
> > vfs_fsync_range(), as I implemented here:
> 
> And I think that's insufficient *also*.
> 
> What you actually want is "sync_file_range()", with the full set of arguments.

That's a different interface. the aio fsync interface has been
exposed to userspace for years, we just haven't implemented it in
the kernel. That's a major difference to everything else being
proposed in this patch set, especially this one.

FYI sync_file_range() is definitely not a fsync/fdatasync
replacement as it does not guarantee data durability in any way.
i.e. you can call sync_file_range, have it wait for data to be
written, return to userspace, then lose power and lose the data that
sync_file_range said it wrote. That's because sync_file_range()
does not:

	a) write the metadata needed to reference the data to disk;
	   and
	b) flush volatile storage caches after data and metadata is
	   written.

Hence sync_file_range is useless to applications that need to
guarantee data durability. Not to mention that most AIO applications
use direct IO, and so have no use for fine grained control over page
cache writeback semantics. They only require a) and b) above, so
implementing the AIO fsync primitive is exactly what they want.

> Yes, really. Sometimes you want to start the writeback, sometimes you
> want to wait for it. Sometimes you want both.

Without durability guarantees such application level optimisations
are pretty much worthless.

> I think this only strengthens my "stop with the idiotic
> special-case-AIO magic already" argument.  If we want something more
> generic than the usual aio, then we should go all in. Not "let's make
> more limited special cases".

No, I don't think this specific case does, because the AIO fsync
interface already exists....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1306941 — Re: [PATCH 07/13] aio: enabled thread based async fsync

On Mon, Jan 11, 2016 at 6:25 PM, Dave Chinner <david@fromorbit.com> wrote:
>
> That's a different interface.

So is openat. So is readahead.

My point is that this idiotic "let's expose special cases" must end.
It's broken. It inevitably only exposes a subset of what different
people would want.

Making "aio_read()" and friends a special interface had historical
reasons for it. But expanding willy-nilly on that model does not.

               Linus

[toc] | [prev] | [next] | [standalone]

#1306967 — Re: [PATCH 07/13] aio: enabled thread based async fsync

On Mon, Jan 11, 2016 at 06:38:15PM -0800, Linus Torvalds wrote:
> On Mon, Jan 11, 2016 at 6:25 PM, Dave Chinner <david@fromorbit.com> wrote:
> >
> > That's a different interface.
> 
> So is openat. So is readahead.
>
> My point is that this idiotic "let's expose special cases" must end.
> It's broken. It inevitably only exposes a subset of what different
> people would want.
> 
> Making "aio_read()" and friends a special interface had historical
> reasons for it. But expanding willy-nilly on that model does not.

Yes, I heard you the first time, but you haven't acknowledged that
the aio fsync interface is indeed different because it already
exists. What's the problem with implementing an AIO call that we've
advertised as supported for many years now that people are asking us
to implement it?

As for a generic async syscall interface, why not just add
IOCB_CMD_SYSCALL that encodes the syscall number and parameters
into the iovec structure and let the existing aio subsystem handle
demultiplexing it and handing them off to threads/workqueues/etc?
That was we get contexts, events, signals, completions,
cancelations, etc from the existing infrastructure, and there's
really only a dispatch/collection layer that needs to be added?

If we then provide the userspace interface via the libaio library to
call the async syscalls with an AIO context handle, then there's
little more that needs to be done to support just about everything
as an async syscall...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1306974 — Re: [PATCH 07/13] aio: enabled thread based async fsync

On Mon, Jan 11, 2016 at 7:37 PM, Dave Chinner <david@fromorbit.com> wrote:
>
> Yes, I heard you the first time, but you haven't acknowledged that
> the aio fsync interface is indeed different because it already
> exists. What's the problem with implementing an AIO call that we've
> advertised as supported for many years now that people are asking us
> to implement it?

Oh, I don't disagree with that. I think it should be exposed, my point
was that that too was not enough.

I don't see why you argue. You said "that's not enough". And I jjust
said that your expansion wasn't sufficient either, and that I think we
should strive to expand things even more.

And preferably not in some ad-hoc manner. Expand it to *everything* we can do.

> As for a generic async syscall interface, why not just add
> IOCB_CMD_SYSCALL that encodes the syscall number and parameters
> into the iovec structure and let the existing aio subsystem handle
> demultiplexing it and handing them off to threads/workqueues/etc?

That would likely be the simplest approach, yes.

There's a few arguments against it, though:

 - doing the indirect system call thing does end up being
architecture-specific, so now you do need the AIO code to call into
some arch wrapper.

   Not a huge deal, since the arch wrapper will be pretty simple (and
we can have a default one that just returns ENOSYS, so that we don't
have to synchronize all architectures)

 - the aio interface really is horrible crap. Really really.

   For example, the whole "send signal as a completion model" is so
f*cking broken that I really don't want to extend the aio interface
too much. I think it's unfixable.

So I really think we'd be *much* better off with a new interface
entirely - preferably one that allows the old aio interfaces to fall
out fairly naturally.

Ben mentioned lio_listio() as a reason for why he wanted to extend the
AIO interface, but I think it works the other way around: yes, we
should look at lio_listio(), but we should look at it mainly as a way
to ask ourselves: "can we implement a new aynchronous system call
submission model that would also make it possible to implement
lio_listio() as a user space wrapper around it".

For example, if we had an actual _good_ way to queue up things, you
could probably make that "struct sigevent" completion for lio_listio()
just be another asynchronous system call at the end of the list - a
system call that sends the completion signal.  And the aiocb_list[]
itself? Maybe those could just be done as normal (individual) aio
calls (so that you end up having the aiocb that you can wait on with
aio_suspend() etc).

But then people who do *not* want the crazy aiocb, and do *not* want
some SIGIO or whatever, could just fire off asynchronous system calls
without that cruddy interface.

So my argument is really that I think it would be better to at least
look into maybe creating something less crapulent, and striving to
make it easy to make the old legacy interfaces be just wrappers around
a more capable model.

And hey, it may be that in the end nobody cares enough, and the right
thing (or at least the prudent thing) to do is to just pile the crap
on deeper and higher, and just add a single IOCB_CMD_SYSCALL
indirection entry.

So I'm not dismissing that as a solution - I just don't think it's a
particularly clean one.

It does have the advantage of likely being a fairly simple hack. But
it smells like a hack.

                Linus

[toc] | [prev] | [next] | [standalone]

#1306984 — Re: [PATCH 07/13] aio: enabled thread based async fsync

On Mon, Jan 11, 2016 at 8:03 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So my argument is really that I think it would be better to at least
> look into maybe creating something less crapulent, and striving to
> make it easy to make the old legacy interfaces be just wrappers around
> a more capable model.

Hmm. Thinking more about this makes me worry about all the system call
versioning and extra work done by libc.

At least glibc has traditionally decided to munge and extend on kernel
system call interfaces, to the point where even fairly core data
structures (like "struct stat") may not always look the same to the
kernel as they do to user space.

So with that worry, I have to admit that maybe a limited interface -
rather than allowing arbitrary generic async system calls - might have
advantages. Less room for mismatches.

I'll have to think about this some more.

                  Linus

[toc] | [prev] | [next] | [standalone]

#1307904 — Re: [PATCH 07/13] aio: enabled thread based async fsync

On Mon, Jan 11, 2016 at 08:48:23PM -0800, Linus Torvalds wrote:
> On Mon, Jan 11, 2016 at 8:03 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > So my argument is really that I think it would be better to at least
> > look into maybe creating something less crapulent, and striving to
> > make it easy to make the old legacy interfaces be just wrappers around
> > a more capable model.
> 
> Hmm. Thinking more about this makes me worry about all the system call
> versioning and extra work done by libc.

That is one of my worries, and one of the reasons an async getdents64() 
or readdir() operation isn't in this batch -- there are a ton of ABI 
issues glibc handles on some platforms.

> At least glibc has traditionally decided to munge and extend on kernel
> system call interfaces, to the point where even fairly core data
> structures (like "struct stat") may not always look the same to the
> kernel as they do to user space.
> 
> So with that worry, I have to admit that maybe a limited interface -
> rather than allowing arbitrary generic async system calls - might have
> advantages. Less room for mismatches.
> 
> I'll have to think about this some more.
> 
>                   Linus

I think some cleanups can be made on how and where the AIO operations
are implemented.  A first stab is below (not very tested as of yet,
still have more work to do) that uses an array to dispatch AIO submits.
By using function pointers to dispatch the operations fairly early in
the process, the code that actually does the required verifications is
less spread out and much easier to follow instead of the giant select
cases.

Another possible improvement might be to move things like aio_fsync()
into sync.c with all the other relevant sync code.  That would make much
more sense and make it much more obvious as to which subsystem
maintainers a given set of functionality really belongs.  If that sounds
like an improvement, I can put some effort into that as well.

		-ben

 aio.c |  242 ++++++++++++++++++++++++++++++++----------------------------------
 1 file changed, 118 insertions(+), 124 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index f776dff..0c06e3b 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -177,6 +177,12 @@ typedef long (*aio_thread_work_fn_t)(struct aio_kiocb *iocb);
  */
 #define KIOCB_CANCELLED		((void *) (~0ULL))
 
+#define AIO_THREAD_NEED_TASK	0x0001	/* Need aio_kiocb->ki_submit_task */
+#define AIO_THREAD_NEED_FS	0x0002	/* Need aio_kiocb->ki_fs */
+#define AIO_THREAD_NEED_FILES	0x0004	/* Need aio_kiocb->ki_files */
+#define AIO_THREAD_NEED_CRED	0x0008	/* Need aio_kiocb->ki_cred */
+#define AIO_THREAD_NEED_MM	0x0010	/* Need the mm context */
+
 struct aio_kiocb {
 	struct kiocb		common;
 
@@ -205,6 +211,7 @@ struct aio_kiocb {
 	struct task_struct	*ki_cancel_task;
 	unsigned long		ki_data;
 	unsigned long		ki_rlimit_fsize;
+	unsigned		ki_thread_flags;	/* AIO_THREAD_NEED... */
 	aio_thread_work_fn_t	ki_work_fn;
 	struct work_struct	ki_work;
 	struct fs_struct	*ki_fs;
@@ -231,16 +238,8 @@ static const struct file_operations aio_ring_fops;
 static const struct address_space_operations aio_ctx_aops;
 
 static void aio_complete(struct kiocb *kiocb, long res, long res2);
-ssize_t aio_fsync(struct kiocb *iocb, int datasync);
-long aio_poll(struct aio_kiocb *iocb);
 
 typedef long (*do_foo_at_t)(int fd, const char *filename, int flags, int mode);
-long aio_do_openat(int fd, const char *filename, int flags, int mode);
-long aio_do_unlinkat(int fd, const char *filename, int flags, int mode);
-long aio_foo_at(struct aio_kiocb *req, do_foo_at_t do_foo_at);
-
-long aio_readahead(struct aio_kiocb *iocb, unsigned long len);
-long aio_renameat(struct aio_kiocb *iocb, struct iocb *user_iocb);
 
 static __always_inline bool aio_may_use_threads(void)
 {
@@ -1533,9 +1532,13 @@ static void aio_thread_fn(struct work_struct *work)
 	old_cancel = cmpxchg(&iocb->ki_cancel,
 			     aio_thread_queue_iocb_cancel_early,
 			     aio_thread_queue_iocb_cancel);
-	if (old_cancel != KIOCB_CANCELLED)
+	if (old_cancel != KIOCB_CANCELLED) {
+		if (iocb->ki_thread_flags & AIO_THREAD_NEED_MM)
+			use_mm(iocb->ki_ctx->mm);
 		ret = iocb->ki_work_fn(iocb);
-	else
+		if (iocb->ki_thread_flags & AIO_THREAD_NEED_MM)
+			unuse_mm(iocb->ki_ctx->mm);
+	} else
 		ret = -EINTR;
 
 	current->kiocb = NULL;
@@ -1566,11 +1569,6 @@ static void aio_thread_fn(struct work_struct *work)
 		flush_signals(current);
 }
 
-#define AIO_THREAD_NEED_TASK	0x0001	/* Need aio_kiocb->ki_submit_task */
-#define AIO_THREAD_NEED_FS	0x0002	/* Need aio_kiocb->ki_fs */
-#define AIO_THREAD_NEED_FILES	0x0004	/* Need aio_kiocb->ki_files */
-#define AIO_THREAD_NEED_CRED	0x0008	/* Need aio_kiocb->ki_cred */
-
 /* aio_thread_queue_iocb
  *	Queues an aio_kiocb for dispatch to a worker thread.  Prepares the
  *	aio_kiocb for cancellation.  The caller must provide a function to
@@ -1581,7 +1579,10 @@ static ssize_t aio_thread_queue_iocb(struct aio_kiocb *iocb,
 				     aio_thread_work_fn_t work_fn,
 				     unsigned flags)
 {
+	if (!aio_may_use_threads())
+		return -EINVAL;
 	INIT_WORK(&iocb->ki_work, aio_thread_fn);
+	iocb->ki_thread_flags = flags;
 	iocb->ki_work_fn = work_fn;
 	if (flags & AIO_THREAD_NEED_TASK) {
 		iocb->ki_submit_task = current;
@@ -1618,7 +1619,6 @@ static long aio_thread_op_read_iter(struct aio_kiocb *iocb)
 	struct file *filp;
 	long ret;
 
-	use_mm(iocb->ki_ctx->mm);
 	filp = iocb->common.ki_filp;
 
 	if (filp->f_op->read_iter) {
@@ -1633,7 +1633,6 @@ static long aio_thread_op_read_iter(struct aio_kiocb *iocb)
 					   filp->f_op->read);
 	else
 		ret = -EINVAL;
-	unuse_mm(iocb->ki_ctx->mm);
 	return ret;
 }
 
@@ -1656,7 +1655,7 @@ ssize_t generic_async_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 		return -EINVAL;
 
 	return aio_thread_queue_iocb(req, aio_thread_op_read_iter,
-				     AIO_THREAD_NEED_TASK);
+				     AIO_THREAD_NEED_TASK | AIO_THREAD_NEED_MM);
 }
 EXPORT_SYMBOL(generic_async_read_iter);
 
@@ -1666,7 +1665,6 @@ static long aio_thread_op_write_iter(struct aio_kiocb *iocb)
 	struct file *filp;
 	long ret;
 
-	use_mm(iocb->ki_ctx->mm);
 	filp = iocb->common.ki_filp;
 	saved_rlim_fsize = rlimit(RLIMIT_FSIZE);
 	current->signal->rlim[RLIMIT_FSIZE].rlim_cur = iocb->ki_rlimit_fsize;
@@ -1684,7 +1682,6 @@ static long aio_thread_op_write_iter(struct aio_kiocb *iocb)
 	else
 		ret = -EINVAL;
 	current->signal->rlim[RLIMIT_FSIZE].rlim_cur = saved_rlim_fsize;
-	unuse_mm(iocb->ki_ctx->mm);
 	return ret;
 }
 
@@ -1708,28 +1705,13 @@ ssize_t generic_async_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 	req->ki_rlimit_fsize = rlimit(RLIMIT_FSIZE);
 
 	return aio_thread_queue_iocb(req, aio_thread_op_write_iter,
-				     AIO_THREAD_NEED_TASK);
+				     AIO_THREAD_NEED_TASK | AIO_THREAD_NEED_MM);
 }
 EXPORT_SYMBOL(generic_async_write_iter);
 
 static long aio_thread_op_fsync(struct aio_kiocb *iocb)
 {
-	return vfs_fsync(iocb->common.ki_filp, 0);
-}
-
-static long aio_thread_op_fdatasync(struct aio_kiocb *iocb)
-{
-	return vfs_fsync(iocb->common.ki_filp, 1);
-}
-
-ssize_t aio_fsync(struct kiocb *iocb, int datasync)
-{
-	struct aio_kiocb *req;
-
-	req = container_of(iocb, struct aio_kiocb, common);
-
-	return aio_thread_queue_iocb(req, datasync ? aio_thread_op_fdatasync
-						   : aio_thread_op_fsync, 0);
+	return vfs_fsync(iocb->common.ki_filp, iocb->ki_data);
 }
 
 static long aio_thread_op_poll(struct aio_kiocb *iocb)
@@ -1766,17 +1748,22 @@ static long aio_thread_op_poll(struct aio_kiocb *iocb)
 	return ret;
 }
 
-long aio_poll(struct aio_kiocb *req)
+static long aio_poll(struct aio_kiocb *req, struct iocb *user_iocb, bool compat)
 {
+	if (!req->common.ki_filp->f_op->poll)
+		return -EINVAL;
+	if ((unsigned short)user_iocb->aio_buf != user_iocb->aio_buf)
+		return -EINVAL;
+	req->ki_data = user_iocb->aio_buf;
 	return aio_thread_queue_iocb(req, aio_thread_op_poll, 0);
 }
 
-long aio_do_openat(int fd, const char *filename, int flags, int mode)
+static long aio_do_openat(int fd, const char *filename, int flags, int mode)
 {
 	return do_sys_open(fd, filename, flags, mode);
 }
 
-long aio_do_unlinkat(int fd, const char *filename, int flags, int mode)
+static long aio_do_unlinkat(int fd, const char *filename, int flags, int mode)
 {
 	if (flags || mode)
 		return -EINVAL;
@@ -1789,7 +1776,6 @@ static long aio_thread_op_foo_at(struct aio_kiocb *req)
 	long ret;
 	u32 fd;
 
-	use_mm(req->ki_ctx->mm);
 	if (unlikely(__get_user(fd, &req->ki_user_iocb->aio_fildes)))
 		ret = -EFAULT;
 	else if (unlikely(__get_user(buf, &req->ki_user_iocb->aio_buf)))
@@ -1804,15 +1790,25 @@ static long aio_thread_op_foo_at(struct aio_kiocb *req)
 				(int)offset,
 				(unsigned short)(offset >> 32));
 	}
-	unuse_mm(req->ki_ctx->mm);
 	return ret;
 }
 
-long aio_foo_at(struct aio_kiocb *req, do_foo_at_t do_foo_at)
+static long aio_openat(struct aio_kiocb *req, struct iocb *uiocb, bool compat)
 {
-	req->ki_data = (unsigned long)(void *)do_foo_at;
+	req->ki_data = (unsigned long)(void *)aio_do_openat;
 	return aio_thread_queue_iocb(req, aio_thread_op_foo_at,
 				     AIO_THREAD_NEED_TASK |
+				     AIO_THREAD_NEED_MM |
+				     AIO_THREAD_NEED_FILES |
+				     AIO_THREAD_NEED_CRED);
+}
+
+static long aio_unlink(struct aio_kiocb *req, struct iocb *uiocb, bool compt)
+{
+	req->ki_data = (unsigned long)(void *)aio_do_unlinkat;
+	return aio_thread_queue_iocb(req, aio_thread_op_foo_at,
+				     AIO_THREAD_NEED_TASK |
+				     AIO_THREAD_NEED_MM |
 				     AIO_THREAD_NEED_FILES |
 				     AIO_THREAD_NEED_CRED);
 }
@@ -1898,17 +1894,23 @@ static long aio_thread_op_readahead(struct aio_kiocb *iocb)
 	return 0;
 }
 
-long aio_readahead(struct aio_kiocb *iocb, unsigned long len)
+static long aio_ra(struct aio_kiocb *iocb, struct iocb *uiocb, bool compat)
 {
 	struct address_space *mapping = iocb->common.ki_filp->f_mapping;
 	pgoff_t index, end;
 	loff_t epos, isize;
 	int do_io = 0;
+	size_t len;
 
+	if (!aio_may_use_threads())
+		return -EINVAL;
+	if (uiocb->aio_buf)
+		return -EINVAL;
 	if (!mapping || !mapping->a_ops)
 		return -EBADF;
 	if (!mapping->a_ops->readpage && !mapping->a_ops->readpages)
 		return -EBADF;
+	len = uiocb->aio_nbytes;
 	if (!len)
 		return 0;
 
@@ -1958,7 +1960,6 @@ static long aio_thread_op_renameat(struct aio_kiocb *iocb)
 	unsigned flags;
 	long ret;
 
-	use_mm(aio_get_mm(&iocb->common));
 	if (unlikely(copy_from_user(&info, user_info, sizeof(info)))) {
 		ret = -EFAULT;
 		goto done;
@@ -1979,39 +1980,47 @@ static long aio_thread_op_renameat(struct aio_kiocb *iocb)
 	else
 		ret = sys_renameat2(olddir, old, newdir, new, flags);
 done:
-	unuse_mm(aio_get_mm(&iocb->common));
 	return ret;
 }
 
-long aio_renameat(struct aio_kiocb *iocb, struct iocb *user_iocb)
+static long aio_rename(struct aio_kiocb *iocb, struct iocb *user_iocb, bool c)
 {
-	const void * __user user_info;
-
 	if (user_iocb->aio_nbytes != sizeof(struct renameat_info))
 		return -EINVAL;
 	if (user_iocb->aio_offset)
 		return -EINVAL;
 
-	user_info = (const void * __user)(long)user_iocb->aio_buf;
-	if (unlikely(!access_ok(VERIFY_READ, user_info,
-				sizeof(struct renameat_info))))
-		return -EFAULT;
-
-	iocb->common.private = (void *)user_info;
+	iocb->common.private = (void *)(long)user_iocb->aio_buf;
 	return aio_thread_queue_iocb(iocb, aio_thread_op_renameat,
 				     AIO_THREAD_NEED_TASK |
+				     AIO_THREAD_NEED_MM |
 				     AIO_THREAD_NEED_FS |
 				     AIO_THREAD_NEED_FILES |
 				     AIO_THREAD_NEED_CRED);
 }
 #endif /* IS_ENABLED(CONFIG_AIO_THREAD) */
 
+long aio_fsync(struct aio_kiocb *req, struct iocb *user_iocb, bool compat)
+{
+	bool datasync = (user_iocb->aio_lio_opcode == IOCB_CMD_FDSYNC);
+	struct file *file = req->common.ki_filp;
+
+	if (file->f_op->aio_fsync)
+		return file->f_op->aio_fsync(&req->common, datasync);
+#if IS_ENABLED(CONFIG_AIO_THREAD)
+	if (file->f_op->fsync) {
+		req->ki_data = datasync;
+		return aio_thread_queue_iocb(req, aio_thread_op_fsync, 0);
+	}
+#endif
+	return -EINVAL;
+}
+
 /*
- * aio_run_iocb:
- *	Performs the initial checks and io submission.
+ * aio_rw:
+ *	Implements read/write vectored and non-vectored
  */
-static ssize_t aio_run_iocb(struct aio_kiocb *req, struct iocb *user_iocb,
-			    bool compat)
+static long aio_rw(struct aio_kiocb *req, struct iocb *user_iocb, bool compat)
 {
 	struct file *file = req->common.ki_filp;
 	ssize_t ret = -EINVAL;
@@ -2085,70 +2094,42 @@ rw_common:
 			file_end_write(file);
 		break;
 
-	case IOCB_CMD_FDSYNC:
-		if (file->f_op->aio_fsync)
-			ret = file->f_op->aio_fsync(&req->common, 1);
-		else if (file->f_op->fsync && (aio_may_use_threads()))
-			ret = aio_fsync(&req->common, 1);
-		break;
-
-	case IOCB_CMD_FSYNC:
-		if (file->f_op->aio_fsync)
-			ret = file->f_op->aio_fsync(&req->common, 0);
-		else if (file->f_op->fsync && (aio_may_use_threads()))
-			ret = aio_fsync(&req->common, 0);
-		break;
-
-	case IOCB_CMD_POLL:
-		if (aio_may_use_threads())
-			ret = aio_poll(req);
-		break;
-
-	case IOCB_CMD_OPENAT:
-		if (aio_may_use_threads())
-			ret = aio_foo_at(req, aio_do_openat);
-		break;
-
-	case IOCB_CMD_UNLINKAT:
-		if (aio_may_use_threads())
-			ret = aio_foo_at(req, aio_do_unlinkat);
-		break;
-
-	case IOCB_CMD_READAHEAD:
-		if (user_iocb->aio_buf)
-			return -EINVAL;
-		if (aio_may_use_threads())
-			ret = aio_readahead(req, user_iocb->aio_nbytes);
-		break;
-
-	case IOCB_CMD_RENAMEAT:
-		if (aio_may_use_threads())
-			ret = aio_renameat(req, user_iocb);
-		break;
-
 	default:
 		pr_debug("EINVAL: no operation provided\n");
-		return -EINVAL;
 	}
+	return ret;
+}
 
-	if (ret != -EIOCBQUEUED) {
-		/*
-		 * There's no easy way to restart the syscall since other AIO's
-		 * may be already running. Just fail this IO with EINTR.
-		 */
-		if (unlikely(ret == -ERESTARTSYS || ret == -ERESTARTNOINTR ||
-			     ret == -ERESTARTNOHAND ||
-			     ret == -ERESTART_RESTARTBLOCK))
-			ret = -EINTR;
-		aio_complete(&req->common, ret, 0);
-	}
+typedef long (*aio_submit_fn_t)(struct aio_kiocb *req, struct iocb *iocb,
+				bool compat);
 
-	return 0;
-}
+#define NEED_FD			0x0001
+
+struct submit_info {
+	aio_submit_fn_t		fn;
+	unsigned long		flags;
+};
+
+static const struct submit_info aio_submit_info[] = {
+	[IOCB_CMD_PREAD]	= { aio_rw,	NEED_FD },
+	[IOCB_CMD_PWRITE]	= { aio_rw,	NEED_FD },
+	[IOCB_CMD_PREADV]	= { aio_rw,	NEED_FD },
+	[IOCB_CMD_PWRITEV]	= { aio_rw,	NEED_FD },
+	[IOCB_CMD_FSYNC]	= { aio_fsync,	NEED_FD },
+	[IOCB_CMD_FDSYNC]	= { aio_fsync,	NEED_FD },
+#if IS_ENABLED(CONFIG_AIO_THREAD)
+	[IOCB_CMD_POLL]		= { aio_poll,	NEED_FD },
+	[IOCB_CMD_OPENAT]	= { aio_openat,	0 },
+	[IOCB_CMD_UNLINKAT]	= { aio_unlink,	0 },
+	[IOCB_CMD_READAHEAD]	= { aio_ra,	NEED_FD },
+	[IOCB_CMD_RENAMEAT]	= { aio_rename,	0 },
+#endif
+};
 
 static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
 			 struct iocb *iocb, bool compat)
 {
+	const struct submit_info *submit_info;
 	struct aio_kiocb *req;
 	ssize_t ret;
 
@@ -2168,23 +2149,26 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
 		return -EINVAL;
 	}
 
+	if (unlikely(iocb->aio_lio_opcode >= ARRAY_SIZE(aio_submit_info)))
+		return -EINVAL;
+	submit_info = &aio_submit_info[iocb->aio_lio_opcode];
+	if (unlikely(!submit_info->fn))
+		return -EINVAL;
+
 	req = aio_get_req(ctx);
 	if (unlikely(!req))
 		return -EAGAIN;
 
-	if (iocb->aio_lio_opcode == IOCB_CMD_OPENAT)
-		req->common.ki_filp = NULL;
-	else {
+	if (submit_info->flags & NEED_FD) {
 		req->common.ki_filp = fget(iocb->aio_fildes);
 		if (unlikely(!req->common.ki_filp)) {
 			ret = -EBADF;
 			goto out_put_req;
 		}
+		req->common.ki_flags = iocb_flags(req->common.ki_filp);
 	}
 	req->common.ki_pos = iocb->aio_offset;
 	req->common.ki_complete = aio_complete;
-	if (req->common.ki_filp)
-		req->common.ki_flags = iocb_flags(req->common.ki_filp);
 
 	if (iocb->aio_flags & IOCB_FLAG_RESFD) {
 		/*
@@ -2212,10 +2196,20 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
 	req->ki_user_iocb = user_iocb;
 	req->ki_user_data = iocb->aio_data;
 
-	ret = aio_run_iocb(req, iocb, compat);
-	if (ret)
-		goto out_put_req;
-
+	ret = submit_info->fn(req, iocb, compat);
+	if (ret != -EIOCBQUEUED) {
+		/*
+		 * There's no easy way to restart the syscall since other AIO's
+		 * may be already running. Just fail this IO with EINTR.
+		 */
+		if (unlikely(ret == -ERESTARTSYS || ret == -ERESTARTNOINTR ||
+			     ret == -ERESTARTNOHAND ||
+			     ret == -ERESTART_RESTARTBLOCK))
+			ret = -EINTR;
+		else if (IS_ERR_VALUE(ret))
+			goto out_put_req;
+		aio_complete(&req->common, ret, 0);
+	}
 	return 0;
 out_put_req:
 	put_reqs_available(ctx, 1);
-- 
"Thought is the essence of where you are now."

[toc] | [prev] | [next] | [standalone]

#1307903 — Re: [PATCH 07/13] aio: enabled thread based async fsync

On Jan 11, 2016 8:04 PM, "Linus Torvalds" <torvalds@linux-foundation.org> wrote:
>
> On Mon, Jan 11, 2016 at 7:37 PM, Dave Chinner <david@fromorbit.com> wrote:
> >
> > Yes, I heard you the first time, but you haven't acknowledged that
> > the aio fsync interface is indeed different because it already
> > exists. What's the problem with implementing an AIO call that we've
> > advertised as supported for many years now that people are asking us
> > to implement it?
>
> Oh, I don't disagree with that. I think it should be exposed, my point
> was that that too was not enough.
>
> I don't see why you argue. You said "that's not enough". And I jjust
> said that your expansion wasn't sufficient either, and that I think we
> should strive to expand things even more.
>
> And preferably not in some ad-hoc manner. Expand it to *everything* we can do.
>
> > As for a generic async syscall interface, why not just add
> > IOCB_CMD_SYSCALL that encodes the syscall number and parameters
> > into the iovec structure and let the existing aio subsystem handle
> > demultiplexing it and handing them off to threads/workqueues/etc?
>
> That would likely be the simplest approach, yes.
>
> There's a few arguments against it, though:
>
>  - doing the indirect system call thing does end up being
> architecture-specific, so now you do need the AIO code to call into
> some arch wrapper.

How many arches *can* do it?  As of 4.4, x86_32 can, but x86_64 can't
yet.  We'd also need a whitelist of acceptable indirect syscalls (e.g.
exit is bad).  And we have to worry about things that depend on the mm
or creds.

It would be extra nice if we could avoid switch_mm for things that
don't need it (fsync) and only do it for things like read that do.

--Andy

[toc] | [prev] | [next] | [standalone]

#1309114 — Re: [PATCH 07/13] aio: enabled thread based async fsync

On 12/01/2016 02:20, Linus Torvalds wrote:
> On Mon, Jan 11, 2016 at 5:11 PM, Dave Chinner <david@fromorbit.com> wrote:
>>
>> Insufficient. Needs the range to be passed through and call
>> vfs_fsync_range(), as I implemented here:
> 
> And I think that's insufficient *also*.
> 
> What you actually want is "sync_file_range()", with the full set of arguments.
> 
> Yes, really. Sometimes you want to start the writeback, sometimes you
> want to wait for it. Sometimes you want both.
> 
> For example, if you are doing your own manual write-behind logic, it
> is not sufficient for "wait for data". What you want is "start IO on
> new data" followed by "wait for old data to have been written out".
> 
> I think this only strengthens my "stop with the idiotic
> special-case-AIO magic already" argument.  If we want something more
> generic than the usual aio, then we should go all in. Not "let's make
> more limited special cases".

The question is, do we really want something more generic than the usual
AIO?

Virt is one of the 10 (that's a binary number) users of AIO, and we
don't even use it by default because in most cases it's really a wash.

Let's compare AIO with a simple userspace thread pool.

AIO has the ability to submit and retrieve the results of multiple
operations at once.  Thread pools do not have the ability to submit
multiple operations at a time (you could play games with FUTEX_WAKE, but
then all the threads in the pool would have cacheline bounces on the futex).

The syscall overhead on the critical path is comparable.  For AIO it's
io_submit+io_getevents, for a thread pool it's FUTEX_WAKE plus invoking
the actual syscall.  Again, the only difference for AIO is batching.

Unless userspace is submitting tens of thousands of operations per
second, which is pretty much the case only for read/write, there's no
real benefit in asynchronous system calls over a userspace thread pool.
 That applies to openat, unlinkat, fadvise (for readahead).  It also
applies to msync and fsync, etc. because if your workload is doing tons
of those you'd better buy yourself a disk with a battery-backed cache,
or an UPS, and remove the msync/fsync altogether.

So I'm really happy if we can move the thread creation overhead for such
a thread pool to the kernel.  It keeps the benefits of batching, it uses
the optimized kernel workqueues, it doesn't incur the cost of pthreads,
it makes it easy to remove the cases where AIO is blocking, it makes it
easy to add support for !O_DIRECT.  But everything else seems overkill.

Paolo

[toc] | [prev] | [next] | [standalone]

#1306904 — Re: [PATCH 07/13] aio: enabled thread based async fsync

On Tue, Jan 12, 2016 at 12:11:28PM +1100, Dave Chinner wrote:
> On Mon, Jan 11, 2016 at 05:07:23PM -0500, Benjamin LaHaise wrote:
> > Enable a fully asynchronous fsync and fdatasync operations in aio using
> > the aio thread queuing mechanism.
> > 
> > Signed-off-by: Benjamin LaHaise <ben.lahaise@solacesystems.com>
> > Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
> 
> Insufficient. Needs the range to be passed through and call
> vfs_fsync_range(), as I implemented here:

Noted.

> https://lkml.org/lkml/2015/10/28/878

Please at least Cc the aio list in the future on aio patches, as I do
not have the time to read linux-kernel these days unless prodded to do
so...

		-ben

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web