[prev in list] [next in list] [prev in thread] [next in thread] 

List:       fuse-devel
Subject:    Re: [fuse-devel] [PATCH] fuse: free fuse_io_priv and return error on async dio request failure
From:       Brian Foster <bfoster () redhat ! com>
Date:       2013-05-30 14:57:32
Message-ID: 51A768DC.9060201 () redhat ! com
[Download RAW message or body]

On 05/30/2013 10:28 AM, Maxim Patlasov wrote:
> 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 <bfoster@redhat.com>
>> ---
>> 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.
> 

Ok, I was initially more worried about the aio_complete() via
fuse_aio_complete() causing a reference count issue than not returning
the error (the result appears to be the same either way). Taking another
look, I see the extra kiocb reference being taken in io_submit_one()
making this safe.

I'll respin a simplified version, thanks for the review...

Brian

> Thanks,
> Maxim


------------------------------------------------------------------------------
Introducing AppDynamics Lite, a free troubleshooting tool for Java/.NET
Get 100% visibility into your production application - at no cost.
Code-level diagnostics for performance bottlenecks with <2% overhead
Download for free and get started troubleshooting in minutes.
http://p.sf.net/sfu/appdyn_d2d_ap1
_______________________________________________
fuse-devel mailing list
fuse-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/fuse-devel

[prev in list] [next in list] [prev in thread] [next in thread]