From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from zeniv.linux.org.uk ([195.92.253.2]:49826 "EHLO
        ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1162515AbeE1OEf (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Mon, 28 May 2018 10:04:35 -0400
Date: Mon, 28 May 2018 15:04:34 +0100
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 1/4] aio: all callers of aio_{read,write,fsync,poll}
 treat 0 and -EIOCBQUEUED the same way
Message-ID: <20180528140434.GX30522@ZenIV.linux.org.uk>
References: <20180527222730.GS30522@ZenIV.linux.org.uk>
 <20180527222853.30715-1-viro@ZenIV.linux.org.uk>
 <20180528051529.GA2806@lst.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180528051529.GA2806@lst.de>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Mon, May 28, 2018 at 07:15:29AM +0200, Christoph Hellwig wrote:
> On Sun, May 27, 2018 at 11:28:50PM +0100, Al Viro wrote:
> > From: Al Viro <viro@zeniv.linux.org.uk>
> > 
> > ... so just make them return 0 when caller does not need to destroy iocb
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> But I think we really need a better name for aio_rw_ret now.
> Unfortunately I can't think of one.

Hell knows...  aio_rw_done(), perhaps?  BTW, I would rather have fput()
in aio_complete_rw() done after ki_list removal - having ->ki_cancel()
callable after fput() is Not Nice(tm).  Consider e.g.
static int ffs_aio_cancel(struct kiocb *kiocb)
{
        struct ffs_io_data *io_data = kiocb->private;
        struct ffs_epfile *epfile = kiocb->ki_filp->private_data;
        int value;

        ENTER();

        spin_lock_irq(&epfile->ffs->eps_lock);

What's to guarantee that kiocb->ki_filp is not already freed and reused by
the time we call that sucker, with its ->private_data pointing to something
completely unrelated?

How about lifting the list removal into aio_complete_rw() and aio_poll_work(),
with WARN_ON() left in its place in aio_complete() itself?  Look:
aio_compelete() call chains are
	aio_complete_rw()
	aio_fsync_work()
	__aio_poll_complete()
		aio_poll_work()
		aio_poll_wake()
		aio_poll()

The call in aio_fsync_work() is guaranteed to have iocb not on cancel lists.
The call in aio_poll_wake() *relies* upon aio_complete() not going into
list removal.  The call in aio_poll() is also guaranteed to be not on cancel
list - we get there only if mask != 0 and we add to ->active_reqs only if
mask == 0.

So if we take the list removal into aio_complete_rw() and aio_poll_wake() we
should get the right ordering - iocb gets removed from the list before fput()
in all cases.  And aio_complete() locking footprint becomes simpler...  As
a fringe benefit, __aio_poll_complete() becomes simply
	fput(req->file);
	aio_complete(iocb, mangle_poll(mask), 0);
since we don't need to order fput() vs. aio_complete() anymore - the caller
of __aio_poll_complete() has already taken care of ->ki_cancel() possibility.