Re: [PATCH 16/22] aio: add support for submission/completion rings

[Christoph Hellwig <hch@infradead.org>]

On Fri, Dec 21, 2018 at 12:22:30PM -0700, Jens Axboe wrote:
> The submission queue (SQ) and completion queue (CQ) rings are shared
> between the application and the kernel. This eliminates the need to
> copy data back and forth to submit and complete IO. We use the same
> structures as the old aio interface. The SQ rings are indexes into a
> struct iocb array, like we would submit through io_submit(), and the
> CQ rings are struct io_event, like we would pass in (and copy back)
> from io_getevents().
> 
> A new system call is added for this, io_ring_enter(). This system call
> submits IO that is stored in the SQ ring, and/or completes IO and stores
> the results in the CQ ring. Hence it's possible to both complete and
> submit IO in a single system call.

So this still reuses the aio interface, but I think that is a really
bad idea.  For one the aio_context_t which really is a chunk of memory
mapped into the user address space really make no sense at all here.

We'd much rather allocate a file descriptor using anon_inode_getfd
and operate on that.  That also means we can just close that fd
instead of needing the magic io_destroy, and save all the checks for
which kind of FD we operate on.

The big question to me is if we really need the polled version of
'classic aio'.  If not we could save io_setup2 and would just need
io_ring_setup and io_ring_enter as the new syscalls, and basically
avoid touching the old aio code entirely.

Also as another potential interface enhancement I think we should
consider pre-registering the files we want to do I/O to in the
io_ring_setup system call, thus avoiding fget/fput entirely
in io_ring_enter.  In general the set of files used for aio-like
operations is rather static and should be known at setup time,
in the worst case we might have to have a version of the setup
call that can modify the set up files (similar to how say
epoll works).

Also this whole API needs a better description, and a CC to the
linux-api list.

> +static void aio_commit_cqring(struct kioctx *ctx, unsigned next_tail)
> +{
> +	struct aio_cq_ring *ring = page_address(ctx->cq_ring.pages[0]);

I don't think we can use page_address here as the memory might be
highmem.  Same for all the other uses of page_address.

> +	range->pages = kzalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);

This should use kcalloc.  Same for a few other instances.

> +static int __io_ring_enter(struct kioctx *ctx, unsigned int to_submit,
> +			   unsigned int min_complete, unsigned int flags)
> +{
> +	int ret = 0;
> +
> +	if (flags & IORING_FLAG_SUBMIT) {
> +		ret = aio_ring_submit(ctx, to_submit);
> +		if (ret < 0)
> +			return ret;
> +	}

I don't think we need the IORING_FLAG_SUBMIT flag - a non-zero
to_submit argument should be a good enough indicator.

Also this interface will need some cache flushing help, othewise
it won't work at all for architectures with VIVT caches.