Re: [fuse-devel] [PATCH] fuse: free fuse_io_priv and return error on async dio request failure

[Maxim P. <mpa...@pa...>]

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