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

On Thu, Jan 20, 2011 at 10:31 AM, Trond Myklebust
[off-list ref] wrote:

On Thu, 2011-01-20 at 10:26 +1100, Nick Piggin wrote:

quoted

On Thu, Jan 20, 2011 at 10:25 AM, Trond Myklebust

quoted

quoted

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?

What would it do?

Just return that zero byte reply to userland.

zero bytes is a valid reply for ordinary read() and write(), so why
should we have to do anything different for aio_read()/aio_write()?

It doesn't give userspace much to do. zero reply from read means
EOF. Zero reply from write is pretty useless, I don't think we do it
in the buffered write path -- we either ensure we write at least
something or have a meaningful error to return.

On Jan 19, 2011, at 6:37 PM, Nick Piggin wrote:

On Thu, Jan 20, 2011 at 10:31 AM, Trond Myklebust
[off-list ref] wrote:

quoted

On Thu, 2011-01-20 at 10:26 +1100, Nick Piggin wrote:

quoted

On Thu, Jan 20, 2011 at 10:25 AM, Trond Myklebust

quoted

quoted

quoted

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?

What would it do?

Just return that zero byte reply to userland.

zero bytes is a valid reply for ordinary read() and write(), so why
should we have to do anything different for aio_read()/aio_write()?

It doesn't give userspace much to do. zero reply from read means
EOF. Zero reply from write is pretty useless, I don't think we do it
in the buffered write path -- we either ensure we write at least
something or have a meaningful error to return.

I think in this case, the zero-length requests are already shunted off.  No zero-length requests make it down here, IIRC.  So we expect that either some bytes are started, or an error occurs.  If zero bytes were started and no error occurred, that's just... wrong.

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

On Thu, Jan 20, 2011 at 10:39 AM, Chuck Lever [off-list ref] wrote:

On Jan 19, 2011, at 6:37 PM, Nick Piggin wrote:

quoted

On Thu, Jan 20, 2011 at 10:31 AM, Trond Myklebust
[off-list ref] wrote:

quoted

On Thu, 2011-01-20 at 10:26 +1100, Nick Piggin wrote:

quoted

On Thu, Jan 20, 2011 at 10:25 AM, Trond Myklebust

quoted

quoted

quoted

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?

What would it do?

Just return that zero byte reply to userland.

zero bytes is a valid reply for ordinary read() and write(), so why
should we have to do anything different for aio_read()/aio_write()?

It doesn't give userspace much to do. zero reply from read means
EOF. Zero reply from write is pretty useless, I don't think we do it
in the buffered write path -- we either ensure we write at least
something or have a meaningful error to return.

I think in this case, the zero-length requests are already shunted off.  No zero-length requests make it down here, IIRC.  So we expect that either some bytes are started, or an error occurs.  If zero bytes were started and no error occurred, that's just... wrong.

Right. So in this case you should have an error to return.

On Thu, 2011-01-20 at 10:37 +1100, Nick Piggin wrote: 

On Thu, Jan 20, 2011 at 10:31 AM, Trond Myklebust
[off-list ref] wrote:

quoted

On Thu, 2011-01-20 at 10:26 +1100, Nick Piggin wrote:

quoted

On Thu, Jan 20, 2011 at 10:25 AM, Trond Myklebust

quoted

quoted

quoted

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?

What would it do?

Just return that zero byte reply to userland.

zero bytes is a valid reply for ordinary read() and write(), so why
should we have to do anything different for aio_read()/aio_write()?

It doesn't give userspace much to do. zero reply from read means
EOF. Zero reply from write is pretty useless, I don't think we do it
in the buffered write path -- we either ensure we write at least
something or have a meaningful error to return.

zero reply from read means EOF _or_ user supplied a zero length buffer.

zero reply from write may also be useless, but it is a valid value. It
can simply mean the user supplied a zero length buffer.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

On Thu, Jan 20, 2011 at 10:49 AM, Trond Myklebust
[off-list ref] wrote:

On Thu, 2011-01-20 at 10:37 +1100, Nick Piggin wrote:

quoted

On Thu, Jan 20, 2011 at 10:31 AM, Trond Myklebust
[off-list ref] wrote:

quoted

On Thu, 2011-01-20 at 10:26 +1100, Nick Piggin wrote:

quoted

On Thu, Jan 20, 2011 at 10:25 AM, Trond Myklebust

quoted

quoted

quoted

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?

What would it do?

Just return that zero byte reply to userland.

zero bytes is a valid reply for ordinary read() and write(), so why
should we have to do anything different for aio_read()/aio_write()?

It doesn't give userspace much to do. zero reply from read means
EOF. Zero reply from write is pretty useless, I don't think we do it
in the buffered write path -- we either ensure we write at least
something or have a meaningful error to return.

zero reply from read means EOF _or_ user supplied a zero length buffer.

zero reply from write may also be useless, but it is a valid value. It
can simply mean the user supplied a zero length buffer.

OK, yes. I'm ignoring zero length request.

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

On Thu, 2011-01-20 at 10:37 +1100, Nick Piggin wrote: 

quoted

On Thu, Jan 20, 2011 at 10:31 AM, Trond Myklebust
[off-list ref] wrote:

quoted

On Thu, 2011-01-20 at 10:26 +1100, Nick Piggin wrote:

quoted

On Thu, Jan 20, 2011 at 10:25 AM, Trond Myklebust

quoted

quoted

quoted

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?

What would it do?

Just return that zero byte reply to userland.

zero bytes is a valid reply for ordinary read() and write(), so why
should we have to do anything different for aio_read()/aio_write()?

It doesn't give userspace much to do. zero reply from read means
EOF. Zero reply from write is pretty useless, I don't think we do it
in the buffered write path -- we either ensure we write at least
something or have a meaningful error to return.

zero reply from read means EOF _or_ user supplied a zero length buffer.

zero reply from write may also be useless, but it is a valid value. It
can simply mean the user supplied a zero length buffer.

This code is in the forward path.  So, here we are just dealing with starting the I/O.  If the server replies with a short read or write, that's handled in the callbacks, not here.

So, -EIO is appropriate if we couldn't even start the I/O, right?

Anyway, this is copied from the existing code, not something new with this patch.  If we want to change this part of the logic, maybe it should be part of a different patch?

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