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@xxxxxxxxxxxxxxxxxx>
> >
> > ... so just make them return 0 when caller does not need to destroy iocb
> >
> > Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
>
> Looks good,
>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
>
> 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.
