From: Maxim P. <mpa...@pa...> - 2013-05-30 14:30:07
|
Hi Brian, The patch looks correct but it (and its description) can be simplified. See please inline comments below. 05/28/2013 11:48 PM, Brian Foster пишет: > The current error handling scheme for async requests depends on the > async request submission taking a reference to the fuse_io_priv. If > an async request is received in fuse_direct_IO() and we fail to > send the request (i.e., get_user_pages() returns -ERESTARTSYS), we > currently end up hung in wait_for_sync_kiocb(). The only thing that really matters (imho) is that we can't use wait_for_sync_kiocb() to wait for async requests because aio_complete() explicitly checks for is_sync_kiocb(iocb) and wakes us up only if it's true. > > Add error handling logic to return the error directly if we have no > fuse requests in flight. The AIO code should handle this error and > complete the iocb itself. Otherwise, set the error on the > fuse_io_priv and either wait for the requests to complete (sync) or > return -EIOCBQUEUED. Let's always return -EIOCBQUEUED for async request. If submission failed, the error will be passed to user via fuse_io_priv. > > Signed-off-by: Brian Foster <bf...@re...> > --- > Hi guys, > > I reproduced this problem by running the aio-dio-invalidate-failure test that's > included as part of xfstests. The test seems to pass, but I suspect the kill() > after the timeout period is what leads to the issue. In short, I receive an > -ERESTARTSYS return from get_user_pages() due to a signal. The error bubbles > back up to this code path and we hit the issue described in the commit log. > > I _think_ this covers all of the various scenarios in this code path (and I'm > not totally sure the locking is necessary). Thoughts? > > Brian > > fs/fuse/file.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index d9f4679..937934f 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -2429,10 +2429,26 @@ fuse_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov, > ret = __fuse_direct_read(io, iov, nr_segs, &pos, count); > > if (io->async) { > + /* > + * If request submission fails completely (no fuse requests in > + * flight), free the io directly and return the error. In the > + * aio case, the aio code will handle the error and complete the > + * iocb. > + */ > + if (ret < 0) { > + spin_lock(&io->lock); > + if (io->reqs == 1) { > + spin_unlock(&io->lock); > + kfree(io); > + return ret; > + } > + spin_unlock(&io->lock); > + } > + We actually needn't this code complication. If something went awry, the user shouldn't care whether to be failed on io_submit() or on io_getevents(). > fuse_aio_complete(io, ret < 0 ? ret : 0, -1); > > /* we have a non-extending, async request, so return */ > - if (ret > 0 && !is_sync_kiocb(iocb)) > + if (!is_sync_kiocb(iocb)) > return -EIOCBQUEUED; > > ret = wait_on_sync_kiocb(iocb); This change alone seems to be enough. Thanks, Maxim |