On Wed, Mar 21, 2018 at 08:32:28AM +0100, Christoph Hellwig wrote:
> With the current aio code there is no need for the magic KIOCB_CANCELLED
> value, as a cancelation just kicks the driver to queue the completion
> ASAP, with all actual completion handling done in another thread. Given
> that both the completion path and cancelation take the context lock there
> is no need for magic cmpxchg loops either.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Acked-by: Jeff Moyer <jmoyer@xxxxxxxxxx>
> ---
> fs/aio.c | 37 +++++++++----------------------------
> 1 file changed, 9 insertions(+), 28 deletions(-)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index c32c315f05b5..2d40cf5dd4ec 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -156,19 +156,6 @@ struct kioctx {
> unsigned id;
> };
>
> -/*
> - * We use ki_cancel == KIOCB_CANCELLED to indicate that a kiocb has been either
> - * cancelled or completed (this makes a certain amount of sense because
> - * successful cancellation - io_cancel() - does deliver the completion to
> - * userspace).
> - *
> - * And since most things don't implement kiocb cancellation and we'd really like
> - * kiocb completion to be lockless when possible, we use ki_cancel to
> - * synchronize cancellation and completion - we only set it to KIOCB_CANCELLED
> - * with xchg() or cmpxchg(), see batch_complete_aio() and kiocb_cancel().
> - */
> -#define KIOCB_CANCELLED ((void *) (~0ULL))
> -
> struct aio_kiocb {
> union {
> struct kiocb rw;
> @@ -565,24 +552,18 @@ void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel)
> }
> EXPORT_SYMBOL(kiocb_set_cancel_fn);
>
> +/*
> + * Only cancel if there ws a ki_cancel function to start with, and we
> + * are the one how managed to clear it (to protect against simulatinious
^^^ ^^^^^^^^^^^^^
I have the same complaint about how/who confusion and the spelling error
in this comment, but otherwise looks fine...
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
> + * cancel calls).
> + */
> static int kiocb_cancel(struct aio_kiocb *kiocb)
> {
> - kiocb_cancel_fn *old, *cancel;
> -
> - /*
> - * Don't want to set kiocb->ki_cancel = KIOCB_CANCELLED unless it
> - * actually has a cancel function, hence the cmpxchg()
> - */
> -
> - cancel = READ_ONCE(kiocb->ki_cancel);
> - do {
> - if (!cancel || cancel == KIOCB_CANCELLED)
> - return -EINVAL;
> -
> - old = cancel;
> - cancel = cmpxchg(&kiocb->ki_cancel, old, KIOCB_CANCELLED);
> - } while (cancel != old);
> + kiocb_cancel_fn *cancel = kiocb->ki_cancel;
>
> + if (!cancel)
> + return -EINVAL;
> + kiocb->ki_cancel = NULL;
> return cancel(&kiocb->rw);
> }
>
> --
> 2.14.2
>
