Re: [PATCH] NFS: Fix "BUG at fs/aio.c:554!"

[Chuck Lever <chuck.lever@oracle.com>]

On Jan 19, 2011, at 6:25 PM, Trond Myklebust wrote:

> On Wed, 2011-01-19 at 17:36 -0500, Chuck Lever wrote: 
>> Nick Piggin reports:
>> 
>>> I'm getting use after frees in aio code in NFS
>>> 
>>> [ 2703.396766] Call Trace:
>>> [ 2703.396858]  [<ffffffff8100b057>] ? native_sched_clock+0x27/0x80
>>> [ 2703.396959]  [<ffffffff8108509e>] ? put_lock_stats+0xe/0x40
>>> [ 2703.397058]  [<ffffffff81088348>] ? lock_release_holdtime+0xa8/0x140
>>> [ 2703.397159]  [<ffffffff8108a2a5>] lock_acquire+0x95/0x1b0
>>> [ 2703.397260]  [<ffffffff811627db>] ? aio_put_req+0x2b/0x60
>>> [ 2703.397361]  [<ffffffff81039701>] ? get_parent_ip+0x11/0x50
>>> [ 2703.397464]  [<ffffffff81612a31>] _raw_spin_lock_irq+0x41/0x80
>>> [ 2703.397564]  [<ffffffff811627db>] ? aio_put_req+0x2b/0x60
>>> [ 2703.397662]  [<ffffffff811627db>] aio_put_req+0x2b/0x60
>>> [ 2703.397761]  [<ffffffff811647fe>] do_io_submit+0x2be/0x7c0
>>> [ 2703.397895]  [<ffffffff81164d0b>] sys_io_submit+0xb/0x10
>>> [ 2703.397995]  [<ffffffff8100307b>] system_call_fastpath+0x16/0x1b
>>> 
>>> Adding some tracing, it is due to nfs completing the request then
>>> returning something other than -EIOCBQUEUED, so aio.c
>>> also completes the request.
>> 
>> To address this, prevent the NFS direct I/O engine from completing
>> async iocbs when the forward path returns an error other than
>> EIOCBQUEUED.
>> 
>> This appears to survive ^C during both "xfstest no. 208" and "fsx -Z."
>> 
>> Cc: Stable <stable@kernel.org>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> 
>> Here's my take.
>> 
>> fs/nfs/direct.c |   32 +++++++++++++++++---------------
>> 1 files changed, 17 insertions(+), 15 deletions(-)
>> 
>> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
>> index e6ace0d..c2176f4 100644
>> --- a/fs/nfs/direct.c
>> +++ b/fs/nfs/direct.c
>> @@ -407,15 +407,16 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
>> 		pos += vec->iov_len;
>> 	}
>> 
>> -	if (put_dreq(dreq))
>> -		nfs_direct_complete(dreq);
>> -
>> -	if (requested_bytes != 0)
>> -		return 0;
>> +	/*
>> +	 * If no bytes were started, return the error, and let the
>> +	 * generic layer handle the completion.
>> +	 */
>> +	if (requested_bytes == 0)
>> +		return result < 0 ? result : -EIO;
>> 
>> -	if (result < 0)
>> -		return result;
>> -	return -EIO;
>> +	if (put_dreq(dreq))
>> +		nfs_direct_write_complete(dreq, dreq->inode);
>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> In nfs_direct_read_schedule_iovec()? Shouldn't that be 
> 		nfs_direct_complete(dreq);

Yes, copy and paste error.  Thanks for the review.

> Also, why is EIO the correct reply when no bytes were read/written? Why
> shouldn't the VFS aio code be able to cope with a zero byte reply?

-EIO is returned only if no other error code has been provided so far.  If no bytes were transferred, normally there is going to be some error code generated by the lower layer.  Should we make this a BUG instead?

A zero-byte write is allowed, of course, but I think that kind of request is shunted off before we arrive here.

> 
>> +	return 0;
>> }
>> 
>> static ssize_t nfs_direct_read(struct kiocb *iocb, const struct iovec *iov,
>> @@ -841,15 +842,16 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
>> 		pos += vec->iov_len;
>> 	}
>> 
>> +	/*
>> +	 * If no bytes were started, return the error, and let the
>> +	 * generic layer handle the completion.
>> +	 */
>> +	if (requested_bytes == 0)
>> +		return result < 0 ? result : -EIO;
>> +
>> 	if (put_dreq(dreq))
>> 		nfs_direct_write_complete(dreq, dreq->inode);
>> -
>> -	if (requested_bytes != 0)
>> -		return 0;
>> -
>> -	if (result < 0)
>> -		return result;
>> -	return -EIO;
>> +	return 0;
>> }
>> 
>> static ssize_t nfs_direct_write(struct kiocb *iocb, const struct iovec *iov,
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer
> 
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com
> 

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com