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

[Jens Axboe <axboe@kernel.dk>]

On 12/27/18 6:47 AM, Christoph Hellwig wrote:
> 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.

I don't think that's a big deal at all, the new ring interface just ends
up being a subset of it. We don't map the old user ring, for instance.

> 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.

I'm not against doing something else for setup, but we still need some
way of passing in information about what we want. Things like ring
sizing. The actual rings we could mmap from known offsets if we went the
anon route. 

> 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.

Don't feel strongly about that part. I do like to continue using the aio
infrastructure instead of rewriting everything. And that means that
regular aio gets polling for free, essentially. So seems kind of silly
NOT to offer it as an option.

> 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).

I agree, that would be a nice improvement to not have to get/put files
all the time.

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

Yep

>> +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.

Made changes to support that.

>> +	range->pages = kzalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
> 
> This should use kcalloc.  Same for a few other instances.

Done

>> +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.

Made that change.

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

Fixed this up too.

-- 
Jens Axboe