From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from verein.lst.de ([213.95.11.211]:58427 "EHLO newverein.lst.de"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1730404AbeLULZe (ORCPT <rfc822;linux-fsdevel@vger.kernel.org>);
        Fri, 21 Dec 2018 06:25:34 -0500
Date: Fri, 21 Dec 2018 12:25:33 +0100
From: Christoph Hellwig <hch@lst.de>
To: Jens Axboe <axboe@kernel.dk>
Cc: linux-fsdevel@vger.kernel.org, linux-aio@kvack.org, hch@lst.de,
        viro@zeniv.linux.org.uk
Subject: Re: [PATCH 07/22] aio: support for IO polling
Message-ID: <20181221112533.GC7319@lst.de>
References: <20181218154230.3120-1-axboe@kernel.dk> <20181218154230.3120-8-axboe@kernel.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181218154230.3120-8-axboe@kernel.dk>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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.

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

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

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