From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp2120.oracle.com ([156.151.31.85]:41712 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934168AbeCTAZQ (ORCPT ); Mon, 19 Mar 2018 20:25:16 -0400 Date: Mon, 19 Mar 2018 17:25:07 -0700 From: "Darrick J. Wong" To: Christoph Hellwig Cc: viro@zeniv.linux.org.uk, Avi Kivity , linux-aio@kvack.org, linux-fsdevel@vger.kernel.org, netdev@vger.kernel.org, linux-api@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 05/36] aio: simplify cancellation Message-ID: <20180320002507.GC7282@magnolia> References: <20180305212743.16664-1-hch@lst.de> <20180305212743.16664-6-hch@lst.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180305212743.16664-6-hch@lst.de> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, Mar 05, 2018 at 01:27:12PM -0800, 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 > Acked-by: Jeff Moyer > --- > 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 "...are the one who managed to clear it (to protect against simultaneous cancel calls)." ? Really only complaining because who/how are both English words... Reviewed-by: Darrick J. Wong --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 >