[PATCH] aio: Do not return ERESTARTSYS as a result of AIO (v2)

[PATCH] aio: Do not return ERESTARTSYS as a result of AIO (v2)

[Jan Kara]

OCFS2 can return ERESTARTSYS from its write function when the process is
signalled while waiting for a cluster lock (and the filesystem is mounted with
intr mount option). Generally, it seems reasonable to allow filesystems to
return this error code from its IO functions. As we must not leak ERESTARTSYS
(and similar error codes) to userspace as a result of an AIO operation, we
have to properly convert it to EINTR inside AIO code (restarting the syscall
isn't really an option because other AIO could have been already submitted by
the same io_submit syscall).

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/aio.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

  Andrew, I'm not sure what happened with the old version of the patch (I had
a feeling you carried it in -mm but I cannot see it there now). Anyway, here's
a new version with better changelog as several people raised questions about
the previous one. Please consider merging it. Thanks.

diff --git a/fs/aio.c b/fs/aio.c
index 3006b5b..b73c9b5 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -712,8 +712,16 @@ static ssize_t aio_run_iocb(struct kiocb *iocb)
 	 */
 	ret = retry(iocb);
 
-	if (ret != -EIOCBRETRY && ret != -EIOCBQUEUED)
+	if (ret != -EIOCBRETRY && ret != -EIOCBQUEUED) {
+		/*
+		 * There's no easy way to restart the syscall since other AIO's
+		 * may be already running. Just fail this IO with EINTR.
+		 */
+		if (unlikely(ret == -ERESTARTSYS || ret == -ERESTARTNOINTR ||
+			     ret == -ERESTARTNOHAND || ret == -ERESTART_RESTARTBLOCK))
+			ret = -EINTR;
 		aio_complete(iocb, ret, 0);
+	}
 out:
 	spin_lock_irq(&ctx->ctx_lock);
 
-- 
1.6.4.2

Re: [PATCH] aio: Do not return ERESTARTSYS as a result of AIO (v2)

[Jeff Moyer]

Jan Kara <jack@suse.cz> writes:

> OCFS2 can return ERESTARTSYS from its write function when the process is
> signalled while waiting for a cluster lock (and the filesystem is mounted with
> intr mount option). Generally, it seems reasonable to allow filesystems to
> return this error code from its IO functions. As we must not leak ERESTARTSYS
> (and similar error codes) to userspace as a result of an AIO operation, we
> have to properly convert it to EINTR inside AIO code (restarting the syscall
> isn't really an option because other AIO could have been already submitted by
> the same io_submit syscall).

I had responded to your last posting with:

  This is a confusing problem description.  First, I'm assuming you
  actually experienced this, is that right?  Care to share the details of
  that?

The above paragraph I think you addressed.  I'm still curious about
this, though.  The submission path should be as non-blocking as
possible.  I guess you can't really get around locking, though.  Was the
problem witnessed when doing O_DIRECT or buffered AIO?

  Next, assuming we can get ERSTARTSYS and friends, it will be the return
  code of a single iocb (reaped via io_getevents), not the return code of
  the io_submit system call.  I'm not saying this is right, I'm just
  saying that your description of the problem is misleading.

That objection stands, but just warrants correcting the problem description.

Anyway, the code itself looks fine.  ;-)

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Re: [PATCH] aio: Do not return ERESTARTSYS as a result of AIO (v2)

[Jan Kara]

On Thu 09-09-10 15:49:19, Jeff Moyer wrote:
> Jan Kara <jack@suse.cz> writes:
> 
> > OCFS2 can return ERESTARTSYS from its write function when the process is
> > signalled while waiting for a cluster lock (and the filesystem is mounted with
> > intr mount option). Generally, it seems reasonable to allow filesystems to
> > return this error code from its IO functions. As we must not leak ERESTARTSYS
> > (and similar error codes) to userspace as a result of an AIO operation, we
> > have to properly convert it to EINTR inside AIO code (restarting the syscall
> > isn't really an option because other AIO could have been already submitted by
> > the same io_submit syscall).
> 
> I had responded to your last posting with:
> 
>   This is a confusing problem description.  First, I'm assuming you
>   actually experienced this, is that right?  Care to share the details of
>   that?
> 
> The above paragraph I think you addressed.  I'm still curious about
> this, though.  The submission path should be as non-blocking as
> possible.  I guess you can't really get around locking, though.  Was the
> problem witnessed when doing O_DIRECT or buffered AIO?
> 
>   Next, assuming we can get ERSTARTSYS and friends, it will be the return
>   code of a single iocb (reaped via io_getevents), not the return code of
>   the io_submit system call.  I'm not saying this is right, I'm just
>   saying that your description of the problem is misleading.
> 
> That objection stands, but just warrants correcting the problem description.
  Hmm, I tried to address that by saying:

As we must not leak ERESTARTSYS (and similar error codes) to userspace
as a result of an AIO operation
     ^^^^^^^^^^^^^^^^^^^^^^^^^^
by which I meant the result value in the iocb structure. But apparently
it's not explicit enough. So would you be happier with something like
"result received via io_getevents() syscall"?

> Anyway, the code itself looks fine.  ;-)
> 
> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
  Thanks.

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] aio: Do not return ERESTARTSYS as a result of AIO (v2)

[Jeff Moyer]

Jan Kara <jack@suse.cz> writes:

> On Thu 09-09-10 15:49:19, Jeff Moyer wrote:
>> Jan Kara <jack@suse.cz> writes:

>>   Next, assuming we can get ERSTARTSYS and friends, it will be the return
>>   code of a single iocb (reaped via io_getevents), not the return code of
>>   the io_submit system call.  I'm not saying this is right, I'm just
>>   saying that your description of the problem is misleading.
>> 
>> That objection stands, but just warrants correcting the problem description.
>   Hmm, I tried to address that by saying:
>
> As we must not leak ERESTARTSYS (and similar error codes) to userspace
> as a result of an AIO operation
>      ^^^^^^^^^^^^^^^^^^^^^^^^^^
> by which I meant the result value in the iocb structure. But apparently
> it's not explicit enough. So would you be happier with something like
> "result received via io_getevents() syscall"?

That part looks fine, no need to change it.  What tripped me up was this: 

  (restarting the syscall isn't really an option because other AIO could
  have been already submitted by the same io_submit syscall)

To me, that sounded like the result would be seen by io_submit instead
of io_getevents.

Cheers,
Jeff