[PATCH] aio updates

[PATCH] aio updates

[John Myers]

Please apply.

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.850   -> 1.851  
#	            fs/aio.c	1.22    -> 1.23   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/10/14	jgmyers@netscape.com	1.851
# aio updates
#     fix uninitialized variable causing incorrect timeout
#     add support for IO_CMD_NOOP
#     make sys_io_cancel(), not cancel method, initialize most of returned result
#     minor aio_cancel_all() optimization
#     fix a debug printk
# --------------------------------------------
#
diff -Nru a/fs/aio.c b/fs/aio.c
--- a/fs/aio.c	Mon Oct 14 16:51:45 2002
+++ b/fs/aio.c	Mon Oct 14 16:51:45 2002
@@ -248,7 +248,7 @@
 	write_unlock(&mm->ioctx_list_lock);
 
 	dprintk("aio: allocated ioctx %p[%ld]: mm=%p mask=0x%x\n",
-		ctx, ctx->user_id, current->mm, ctx->ring_info.ring->nr);
+		ctx, ctx->user_id, current->mm, ctx->ring_info.nr);
 	return ctx;
 
 out_cleanup:
@@ -281,12 +281,12 @@
 		struct kiocb *iocb = list_kiocb(pos);
 		list_del_init(&iocb->ki_list);
 		cancel = iocb->ki_cancel;
-		if (cancel)
+		if (cancel) {
 			iocb->ki_users++;
-		spin_unlock_irq(&ctx->ctx_lock);
-		if (cancel)
+			spin_unlock_irq(&ctx->ctx_lock);
 			cancel(iocb, &res);
-		spin_lock_irq(&ctx->ctx_lock);
+			spin_lock_irq(&ctx->ctx_lock);
+		}
 	}
 	spin_unlock_irq(&ctx->ctx_lock);
 }
@@ -845,13 +845,13 @@
 
 	/* End fast path */
 
+	init_timeout(&to);
 	if (timeout) {
 		struct timespec	ts;
 		ret = -EFAULT;
 		if (unlikely(copy_from_user(&ts, timeout, sizeof(ts))))
 			goto out;
 
-		init_timeout(&to);
 		set_timeout(start_jiffies, &to, &ts);
 	}
 
@@ -1073,6 +1073,9 @@
 		if (file->f_op->aio_fsync)
 			ret = file->f_op->aio_fsync(req, 0);
 		break;
+	case IOCB_CMD_NOOP:
+		aio_complete(req, iocb->aio_buf, iocb->aio_offset);
+		return 0;
 	default:
 		dprintk("EINVAL: io_submit: no operation provided\n");
 		ret = -EINVAL;
@@ -1197,6 +1200,9 @@
 	if (NULL != cancel) {
 		struct io_event tmp;
 		printk("calling cancel\n");
+		memset(&tmp, 0, sizeof(tmp));
+		tmp.obj = (u64)(unsigned long)kiocb->ki_user_obj;
+		tmp.data = kiocb->ki_user_data;
 		ret = cancel(kiocb, &tmp);
 		if (!ret) {
 			/* Cancellation succeeded -- copy the result

Re: [PATCH] aio updates

[Benjamin LaHaise]

On Mon, Oct 14, 2002 at 06:17:33PM -0700, John Myers wrote:
> Please apply. 
> #     fix uninitialized variable causing incorrect timeout
> #     add support for IO_CMD_NOOP
> #     make sys_io_cancel(), not cancel method, initialize most of returned result
> #     minor aio_cancel_all() optimization
> #     fix a debug printk

I've applied most of this to my tree, except for the NOOP bits.  My 
concern is that the way you've implemented NOOP does not allow for all 
possible return codes to be passed in due to the error checking the 
iocb submit code performs on the inputs.  It can also spuriously fail 
if the filedescriptor field points to an fd that doesn't exist, which 
could be somewhat unexpected (especially if it is initialized to 0 by 
default, and therefore would not fail during normal operation where a 
program is run with stdin).  Got any idea for cleaning those problems 
up?

		-ben

Re: [PATCH] aio updates

[John Gardiner Myers]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

Benjamin LaHaise wrote:

>My 
>concern is that the way you've implemented NOOP does not allow for all 
>possible return codes to be passed in due to the error checking the 
>iocb submit code performs on the inputs.
>
Could you provide an example of a possible return code that cannot be 
passed in?  I know you can't pass a 64 bit return code on a 32 bit 
platform, but then neither can any other operation.

>It can also spuriously fail 
>if the filedescriptor field points to an fd that doesn't exist,
>
Currently the operation requires a valid fd just like every other 
operation does, so I don't consider such a failure to be spurious.

The alternative is to change the aio framework itself to support 
operations that don't work on fds.  That would move the fget() call and 
the overflow check to below where it sets req->ki_user_data.  The check 
for IOCB_CMD_NOOP would then go before the fget() call and overflow check.

If you think this is the way to go, I can code up patch to do this.


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3537 bytes --]

Re: [PATCH] aio updates

[Benjamin LaHaise]

On Tue, Oct 15, 2002 at 01:50:40PM -0700, John Gardiner Myers wrote:
> Benjamin LaHaise wrote:
> 
> >My 
> >concern is that the way you've implemented NOOP does not allow for all 
> >possible return codes to be passed in due to the error checking the 
> >iocb submit code performs on the inputs.
> >
> Could you provide an example of a possible return code that cannot be 
> passed in?  I know you can't pass a 64 bit return code on a 32 bit 
> platform, but then neither can any other operation.

aio_nbytes is clamped to disallow negative values at present.

> Currently the operation requires a valid fd just like every other 
> operation does, so I don't consider such a failure to be spurious.

Okay, if it is documented, that makes sense.  I just wasn't sure if it 
happened to be an oversight.

> The alternative is to change the aio framework itself to support 
> operations that don't work on fds.  That would move the fget() call and 
> the overflow check to below where it sets req->ki_user_data.  The check 
> for IOCB_CMD_NOOP would then go before the fget() call and overflow check.
> 
> If you think this is the way to go, I can code up patch to do this.

That's a possibility.  I don't like the idea of splitting things out too 
much, as special cases look ugly.  The io_submit interface really does 
assume that the file descriptor exists, and removing that doesn't really 
make sense -- non-file descriptor consuming operations should most likely 
be their own syscalls (imho).  But if you're okay with requiring an fd 
for the NOOP, then that seems alright.  Does anyone else have any thoughts 
on the matter?

		-ben
-- 
"Do you seek knowledge in time travel?"

Re: [PATCH] aio updates

[John Gardiner Myers]

[-- Attachment #1: Type: text/plain, Size: 143 bytes --]

Benjamin LaHaise wrote:

>aio_nbytes is clamped to disallow negative values at present.
>  
>
The IOCB_CMD_NOOP patch doesn't use aio_nbytes.


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3537 bytes --]

Re: [PATCH] aio updates

[John Gardiner Myers]

[-- Attachment #1: Type: text/plain, Size: 299 bytes --]

Benjamin LaHaise wrote:

>non-file descriptor consuming operations should most likely 
>be their own syscalls (imho).
>
This would defeat the multiple iocb submission feature of io_submit(). 
 All async operations should be submittable through io_submit() to 
permit multiple operation submission.


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3537 bytes --]