On 12/21/18 4:25 AM, Christoph Hellwig wrote:
> On Tue, Dec 18, 2018 at 08:42:15AM -0700, Jens Axboe wrote:
>> Add polled variants of PREAD/PREADV and PWRITE/PWRITEV. These act
>> like their non-polled counterparts, except we expect to poll for
>> completion of them. The polling happens at io_getevent() time, and
>> works just like non-polled IO.
>
> This description is highly misleading as it doesn't match the code.
>
> Also needs a Cc to linux-api and a man page addition.
>
>> +#define AIO_IOPOLL_BATCH 8
>
> We later rename this to AIO_POLL batch and move it up the file,
> it would be great if we could that fold in here.
It is moved, but it's never renamed. I'll get rid of the move.
>> + /*
>> + * Handle EAGAIN from resource limits with polled IO inline, don't
>> + * pass the event back to userspace.
>> + */
>
> This comment looks confusing. I think this is the RWF_NOWAIT failure?
> I'd say either explain it way better or not at all.
Right, it's for RQF_NOWAIT. The point is we return that in the
submission path, not through an io_event. I'll update the comment.
>> + if (unlikely(res == -EAGAIN))
>> + set_bit(KIOCB_F_POLL_EAGAIN, &iocb->ki_flags);
>> + else {
>> + aio_fill_event(&iocb->ki_ev, iocb, res, res2);
>> + set_bit(KIOCB_F_POLL_COMPLETED, &iocb->ki_flags);
>> + }
>
> Please always use braces on both sides of the else.
Fixed
>> + if (iocb->aio_flags & IOCB_FLAG_HIPRI) {
>> + /* shares space in the union, and is rather pointless.. */
>> + ret = -EINVAL;
>> + if (iocb->aio_flags & IOCB_FLAG_RESFD)
>> + goto out_fput;
>> +
>> + /* can't submit polled IO to a non-polled ctx */
>> + if (!(ctx->flags & IOCTX_FLAG_IOPOLL))
>> + goto out_fput;
>
> Given that we can't mix and match polled and non-polled I/O we
> should drop IOCB_FLAG_HIPRI entirely.
I kind of like how it's explicit like that, even if it isn't strictly
needed. It makes it easier to verify apps are doing the right thing too.
Don't feel super strongly about it, though.
--
Jens Axboe
