[PATCH][RFC] AIO: always reinitialize iocb->ki_run_list at the end of aio_run_iocb()

[PATCH][RFC] AIO: always reinitialize iocb->ki_run_list at the end of aio_run_iocb()

[Sergey Temerkhanov]

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

This patch makes aio_run_iocb() to always reinitialize iocb->ki_run_list (not 
only when iocb->ki_retry() function returns -EIOCBRETRY) so that subsequent 
call of kick_iocb() will succeed.

Regards, Sergey Temerkhanov,
Cifronic ZAO.

[-- Attachment #2: reinit-ki_run_list.patch --]
[-- Type: text/x-patch, Size: 656 bytes --]

diff -r 97344a0f62c9 fs/aio.c
--- a/fs/aio.c	Tue Apr 27 21:18:14 2010 +0400
+++ b/fs/aio.c	Tue Apr 27 21:30:23 2010 +0400
@@ -748,6 +748,9 @@
 out:
 	spin_lock_irq(&ctx->ctx_lock);
 
+	/* will make __queue_kicked_iocb succeed from here on */
+	INIT_LIST_HEAD(&iocb->ki_run_list);
+
 	if (-EIOCBRETRY == ret) {
 		/*
 		 * OK, now that we are done with this iteration
@@ -756,8 +759,6 @@
 		 * "kick" can start the next iteration
 		 */
 
-		/* will make __queue_kicked_iocb succeed from here on */
-		INIT_LIST_HEAD(&iocb->ki_run_list);
 		/* we must queue the next iteration ourselves, if it
 		 * has already been kicked */
 		if (kiocbIsKicked(iocb)) {

Re: [PATCH][RFC] AIO: always reinitialize iocb->ki_run_list at the end of aio_run_iocb()

[Andrew Morton]

On Wed, 28 Apr 2010 02:51:43 +0400
Sergey Temerkhanov <temerkhanov@yandex.ru> wrote:

> This patch makes aio_run_iocb() to always reinitialize iocb->ki_run_list (not 
> only when iocb->ki_retry() function returns -EIOCBRETRY) so that subsequent 
> call of kick_iocb() will succeed.
> 
> Regards, Sergey Temerkhanov,
> Cifronic ZAO.
> 
> 
> [reinit-ki_run_list.patch  text/x-patch (657B)]
> diff -r 97344a0f62c9 fs/aio.c
> --- a/fs/aio.c	Tue Apr 27 21:18:14 2010 +0400
> +++ b/fs/aio.c	Tue Apr 27 21:30:23 2010 +0400
> @@ -748,6 +748,9 @@
>  out:
>  	spin_lock_irq(&ctx->ctx_lock);
>  
> +	/* will make __queue_kicked_iocb succeed from here on */
> +	INIT_LIST_HEAD(&iocb->ki_run_list);
> +
>  	if (-EIOCBRETRY == ret) {
>  		/*
>  		 * OK, now that we are done with this iteration
> @@ -756,8 +759,6 @@
>  		 * "kick" can start the next iteration
>  		 */
>  
> -		/* will make __queue_kicked_iocb succeed from here on */
> -		INIT_LIST_HEAD(&iocb->ki_run_list);
>  		/* we must queue the next iteration ourselves, if it
>  		 * has already been kicked */
>  		if (kiocbIsKicked(iocb)) {

I assume that this fixes some runtime problem which you observed?

Can you please describe that problem?  This code is pretty old - what
was your application doing that nobody else's application has thus far
done?

Also, please send your Signed-off-by: for this patch, as per
Documentation/Submittingpatches, thanks.

Re: [PATCH][RFC] AIO: always reinitialize iocb->ki_run_list at the end of aio_run_iocb()

[Sergey Temerkhanov]

On Wednesday 28 April 2010 22:31:49 Andrew Morton wrote:
> On Wed, 28 Apr 2010 02:51:43 +0400
> 
> Sergey Temerkhanov <temerkhanov@yandex.ru> wrote:
> > This patch makes aio_run_iocb() to always reinitialize iocb->ki_run_list
> > (not only when iocb->ki_retry() function returns -EIOCBRETRY) so that
> > subsequent call of kick_iocb() will succeed.
> >
> > Regards, Sergey Temerkhanov,
> > Cifronic ZAO.
> >
> >
> > [reinit-ki_run_list.patch  text/x-patch (657B)]
> > diff -r 97344a0f62c9 fs/aio.c
> > --- a/fs/aio.c	Tue Apr 27 21:18:14 2010 +0400
> > +++ b/fs/aio.c	Tue Apr 27 21:30:23 2010 +0400
> > @@ -748,6 +748,9 @@
> >  out:
> >  	spin_lock_irq(&ctx->ctx_lock);
> >
> > +	/* will make __queue_kicked_iocb succeed from here on */
> > +	INIT_LIST_HEAD(&iocb->ki_run_list);
> > +
> >  	if (-EIOCBRETRY == ret) {
> >  		/*
> >  		 * OK, now that we are done with this iteration
> > @@ -756,8 +759,6 @@
> >  		 * "kick" can start the next iteration
> >  		 */
> >
> > -		/* will make __queue_kicked_iocb succeed from here on */
> > -		INIT_LIST_HEAD(&iocb->ki_run_list);
> >  		/* we must queue the next iteration ourselves, if it
> >  		 * has already been kicked */
> >  		if (kiocbIsKicked(iocb)) {
> 
> I assume that this fixes some runtime problem which you observed?
> 
> Can you please describe that problem?  This code is pretty old - what
> was your application doing that nobody else's application has thus far
> done?

I've written the driver code which implements a zero-copy DMA char device. It 
has aio_read() and aio_write() methods which return -EIOCBQUEUED after the 
successful preparation of the buffers described by kiocb and posting it to the 
descriptor chain. When the descriptors are processed, the DMA engine raises 
the interrupt and the cleanup work is done in the handler, including 
aio_complete() for the completed kiocbs.

This works fine, however, there is a problem with canceling the queued 
requests, espesially on io_destroy() syscall. Since there is no simple way to 
remove single kiocb from the descriptor chain, I'm removing all of them from 
the queue using aio_complete() or aio_put_req() in the ki_cancel() callback 
routine of my driver. The main problem is the reference counting in 
aio_cancel_all():

		if (cancel) {
			iocb->ki_users++;
			spin_unlock_irq(&ctx->ctx_lock);
			cancel(iocb, &res);
			spin_lock_irq(&ctx->ctx_lock);
		}

Here the iocb->ki_users gets incremented which already has the value 1 at this 
point (after the io_submit_one() completion) and it's never released (). So I 
have to call aio_put_req() twice for the given kiocb (this seems to be the 
hack to me) or I'll end up with the unkillable process stuck in 
wait_for_all_aios() at the io_schedule(). I've posted the patches where I've 
added aio_put_req() but I think it needs more testing. So, I've tried another 
approach (hack) - requeue the kiocb with kick_iocb() before calling 
aio_put_req() in the ki_cancel() callback (that's because aio_run_iocb() takes 
some special actions for the canceled kiocbs). And I've found out that 
kick_iocb() fails because aio_run_iocb() does this:
	iocb->ki_run_list.next = iocb->ki_run_list.prev = NULL;
and only reinitializes iocb->ki_run_list when iocb->ki_retry() returns 
-EIOCBRETRY but kick_iocb() is exported and looks like intended for usage 
(though not recommended).

The only place where I've found the similar approach to AIO in the device 
driver is drivers/usb/gadget/inode.c.

> 
> Also, please send your Signed-off-by: for this patch, as per
> Documentation/Submittingpatches, thanks.
> 

Sorry, I've forgotten this.

Signed-off-by: Sergey Temerkhanov <temerkhanov@cifronik.ru>

Regards, Sergey Temerkhanov,
Cifronic ZAO

Re: [PATCH][RFC] AIO: always reinitialize iocb->ki_run_list at the end of aio_run_iocb()

[Andrew Morton]

On Fri, 30 Apr 2010 02:56:58 +0400
Sergey Temerkhanov <temerkhanov@yandex.ru> wrote:

> On Wednesday 28 April 2010 22:31:49 Andrew Morton wrote:
> > On Wed, 28 Apr 2010 02:51:43 +0400
> > 
> > Sergey Temerkhanov <temerkhanov@yandex.ru> wrote:
> > > This patch makes aio_run_iocb() to always reinitialize iocb->ki_run_list
> > > (not only when iocb->ki_retry() function returns -EIOCBRETRY) so that
> > > subsequent call of kick_iocb() will succeed.
> > >
> > > Regards, Sergey Temerkhanov,
> > > Cifronic ZAO.
> > >
> > >
> > > [reinit-ki_run_list.patch  text/x-patch (657B)]
> > > diff -r 97344a0f62c9 fs/aio.c
> > > --- a/fs/aio.c	Tue Apr 27 21:18:14 2010 +0400
> > > +++ b/fs/aio.c	Tue Apr 27 21:30:23 2010 +0400
> > > @@ -748,6 +748,9 @@
> > >  out:
> > >  	spin_lock_irq(&ctx->ctx_lock);
> > >
> > > +	/* will make __queue_kicked_iocb succeed from here on */
> > > +	INIT_LIST_HEAD(&iocb->ki_run_list);
> > > +
> > >  	if (-EIOCBRETRY == ret) {
> > >  		/*
> > >  		 * OK, now that we are done with this iteration
> > > @@ -756,8 +759,6 @@
> > >  		 * "kick" can start the next iteration
> > >  		 */
> > >
> > > -		/* will make __queue_kicked_iocb succeed from here on */
> > > -		INIT_LIST_HEAD(&iocb->ki_run_list);
> > >  		/* we must queue the next iteration ourselves, if it
> > >  		 * has already been kicked */
> > >  		if (kiocbIsKicked(iocb)) {
> > 
> > I assume that this fixes some runtime problem which you observed?
> > 
> > Can you please describe that problem?  This code is pretty old - what
> > was your application doing that nobody else's application has thus far
> > done?
> 
> I've written the driver code which implements a zero-copy DMA char device. It 
> has aio_read() and aio_write() methods which return -EIOCBQUEUED after the 
> successful preparation of the buffers described by kiocb and posting it to the 
> descriptor chain. When the descriptors are processed, the DMA engine raises 
> the interrupt and the cleanup work is done in the handler, including 
> aio_complete() for the completed kiocbs.
> 
> This works fine, however, there is a problem with canceling the queued 
> requests, espesially on io_destroy() syscall. Since there is no simple way to 
> remove single kiocb from the descriptor chain, I'm removing all of them from 
> the queue using aio_complete() or aio_put_req() in the ki_cancel() callback 
> routine of my driver. The main problem is the reference counting in 
> aio_cancel_all():
> 
> 		if (cancel) {
> 			iocb->ki_users++;
> 			spin_unlock_irq(&ctx->ctx_lock);
> 			cancel(iocb, &res);
> 			spin_lock_irq(&ctx->ctx_lock);
> 		}
> 
> Here the iocb->ki_users gets incremented which already has the value 1 at this 
> point (after the io_submit_one() completion) and it's never released (). So I 
> have to call aio_put_req() twice for the given kiocb (this seems to be the 
> hack to me) or I'll end up with the unkillable process stuck in 
> wait_for_all_aios() at the io_schedule(). I've posted the patches where I've 
> added aio_put_req() but I think it needs more testing. So, I've tried another 
> approach (hack) - requeue the kiocb with kick_iocb() before calling 
> aio_put_req() in the ki_cancel() callback (that's because aio_run_iocb() takes 
> some special actions for the canceled kiocbs). And I've found out that 
> kick_iocb() fails because aio_run_iocb() does this:
> 	iocb->ki_run_list.next = iocb->ki_run_list.prev = NULL;
> and only reinitializes iocb->ki_run_list when iocb->ki_retry() returns 
> -EIOCBRETRY but kick_iocb() is exported and looks like intended for usage 
> (though not recommended).
> 
> The only place where I've found the similar approach to AIO in the device 
> driver is drivers/usb/gadget/inode.c.

Looking up a few lines in aio_run_iocb() I see the helpful comment:

	/*
	 * This is so that aio_complete knows it doesn't need to
	 * pull the iocb off the run list (We can't just call
	 * INIT_LIST_HEAD because we don't want a kick_iocb to
	 * queue this on the run list yet)
	 */
	iocb->ki_run_list.next = iocb->ki_run_list.prev = NULL;

and I wonder whether your change broke that.					

Given that we've already run aoi_complete(), I assume it's OK, but it
would be good if some of the more recently-involved aio guys could haev
a think, please.

--- a/fs/aio.c~aio-always-reinitialize-iocb-ki_run_list-at-the-end-of-aio_run_iocb
+++ a/fs/aio.c
@@ -717,6 +717,9 @@ static ssize_t aio_run_iocb(struct kiocb
 out:
 	spin_lock_irq(&ctx->ctx_lock);
 
+	/* will make __queue_kicked_iocb succeed from here on */
+	INIT_LIST_HEAD(&iocb->ki_run_list);
+
 	if (-EIOCBRETRY == ret) {
 		/*
 		 * OK, now that we are done with this iteration
@@ -725,8 +728,6 @@ out:
 		 * "kick" can start the next iteration
 		 */
 
-		/* will make __queue_kicked_iocb succeed from here on */
-		INIT_LIST_HEAD(&iocb->ki_run_list);
 		/* we must queue the next iteration ourselves, if it
 		 * has already been kicked */
 		if (kiocbIsKicked(iocb)) {
_

Re: [PATCH][RFC] AIO: always reinitialize iocb->ki_run_list at the end of aio_run_iocb()

[Jeff Moyer]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Fri, 30 Apr 2010 02:56:58 +0400
> Sergey Temerkhanov <temerkhanov@yandex.ru> wrote:
>
>> On Wednesday 28 April 2010 22:31:49 Andrew Morton wrote:
>> > On Wed, 28 Apr 2010 02:51:43 +0400
>> > 
>> > Sergey Temerkhanov <temerkhanov@yandex.ru> wrote:
>> > > This patch makes aio_run_iocb() to always reinitialize iocb->ki_run_list
>> > > (not only when iocb->ki_retry() function returns -EIOCBRETRY) so that
>> > > subsequent call of kick_iocb() will succeed.
>> > >
>> > > Regards, Sergey Temerkhanov,
>> > > Cifronic ZAO.
>> > >
>> > >
>> > > [reinit-ki_run_list.patch  text/x-patch (657B)]
>> > > diff -r 97344a0f62c9 fs/aio.c
>> > > --- a/fs/aio.c	Tue Apr 27 21:18:14 2010 +0400
>> > > +++ b/fs/aio.c	Tue Apr 27 21:30:23 2010 +0400
>> > > @@ -748,6 +748,9 @@
>> > >  out:
>> > >  	spin_lock_irq(&ctx->ctx_lock);
>> > >
>> > > +	/* will make __queue_kicked_iocb succeed from here on */
>> > > +	INIT_LIST_HEAD(&iocb->ki_run_list);
>> > > +
>> > >  	if (-EIOCBRETRY == ret) {
>> > >  		/*
>> > >  		 * OK, now that we are done with this iteration
>> > > @@ -756,8 +759,6 @@
>> > >  		 * "kick" can start the next iteration
>> > >  		 */
>> > >
>> > > -		/* will make __queue_kicked_iocb succeed from here on */
>> > > -		INIT_LIST_HEAD(&iocb->ki_run_list);
>> > >  		/* we must queue the next iteration ourselves, if it
>> > >  		 * has already been kicked */
>> > >  		if (kiocbIsKicked(iocb)) {
>> > 
>> > I assume that this fixes some runtime problem which you observed?
>> > 
>> > Can you please describe that problem?  This code is pretty old - what
>> > was your application doing that nobody else's application has thus far
>> > done?
>> 
>> I've written the driver code which implements a zero-copy DMA char device. It 
>> has aio_read() and aio_write() methods which return -EIOCBQUEUED after the 
>> successful preparation of the buffers described by kiocb and posting it to the 
>> descriptor chain. When the descriptors are processed, the DMA engine raises 
>> the interrupt and the cleanup work is done in the handler, including 
>> aio_complete() for the completed kiocbs.
>> 
>> This works fine, however, there is a problem with canceling the queued 
>> requests, espesially on io_destroy() syscall. Since there is no simple way to 
>> remove single kiocb from the descriptor chain, I'm removing all of them from 
>> the queue using aio_complete() or aio_put_req() in the ki_cancel() callback 
>> routine of my driver. The main problem is the reference counting in 
>> aio_cancel_all():
>> 
>> 		if (cancel) {
>> 			iocb->ki_users++;
>> 			spin_unlock_irq(&ctx->ctx_lock);
>> 			cancel(iocb, &res);
>> 			spin_lock_irq(&ctx->ctx_lock);
>> 		}
>> 
>> Here the iocb->ki_users gets incremented which already has the value 1 at this 
>> point (after the io_submit_one() completion) and it's never released (). So I 
>> have to call aio_put_req() twice for the given kiocb (this seems to be the 
>> hack to me) or I'll end up with the unkillable process stuck in 
>> wait_for_all_aios() at the io_schedule(). I've posted the patches where I've 
>> added aio_put_req() but I think it needs more testing.

I can vaguely recall discussion surrounding the reference counting of
cancel methods, but I have no idea what the actual contents of those
discussions were.  Sorry, my memory has failed me.  Either Zach or
Suparna might remember better.

Sergey, the cancellation path, unfortunately, is not well exercised as
I'm sure you are aware.  As you pointed out, the only implementation of
a cancel method is the usb gadget interface.  Now, given that they've
worked fine with the extra put in their cancel method, I'm not sure why
you can't do the same.  It sure is easier to validate than what you
propose below:

>> So, I've tried another 
>> approach (hack) - requeue the kiocb with kick_iocb() before calling 
>> aio_put_req() in the ki_cancel() callback (that's because aio_run_iocb() takes 
>> some special actions for the canceled kiocbs). And I've found out that 
>> kick_iocb() fails because aio_run_iocb() does this:
>> 	iocb->ki_run_list.next = iocb->ki_run_list.prev = NULL;
>> and only reinitializes iocb->ki_run_list when iocb->ki_retry() returns 
>> -EIOCBRETRY but kick_iocb() is exported and looks like intended for usage 
>> (though not recommended).
>> 
>> The only place where I've found the similar approach to AIO in the device 
>> driver is drivers/usb/gadget/inode.c.
>
> Looking up a few lines in aio_run_iocb() I see the helpful comment:
>
> 	/*
> 	 * This is so that aio_complete knows it doesn't need to
> 	 * pull the iocb off the run list (We can't just call
> 	 * INIT_LIST_HEAD because we don't want a kick_iocb to
> 	 * queue this on the run list yet)
> 	 */
> 	iocb->ki_run_list.next = iocb->ki_run_list.prev = NULL;
>
> and I wonder whether your change broke that.					
>
> Given that we've already run aoi_complete(), I assume it's OK, but it
> would be good if some of the more recently-involved aio guys could haev
> a think, please.

I'd rather not muddy these waters even further.  Unless there is a
compelling reason why the patch author cannot do an extra put in his
cancel method, I'd say that is the best way forward.

Cheers,
Jeff

Re: [PATCH][RFC] AIO: always reinitialize iocb->ki_run_list at the end of aio_run_iocb()

[Sergey Temerkhanov]

On Wednesday 26 May 2010 23:38:35 Jeff Moyer wrote:
...
> I can vaguely recall discussion surrounding the reference counting of
> cancel methods, but I have no idea what the actual contents of those
> discussions were.  Sorry, my memory has failed me.  Either Zach or
> Suparna might remember better.
> 
> Sergey, the cancellation path, unfortunately, is not well exercised as
> I'm sure you are aware.  As you pointed out, the only implementation of
> a cancel method is the usb gadget interface.  Now, given that they've
> worked fine with the extra put in their cancel method, I'm not sure why
> you can't do the same. 
Well, in fact, they have only one aio_put_req() in their cancel method. This 
is the code from 2.6.34:

static int ep_aio_cancel(struct kiocb *iocb, struct io_event *e)
{
	struct kiocb_priv	*priv = iocb->private;
	struct ep_data		*epdata;
	int			value;

	local_irq_disable();
	epdata = priv->epdata;
	// spin_lock(&epdata->dev->lock);
	kiocbSetCancelled(iocb);
	if (likely(epdata && epdata->ep && priv->req))
		value = usb_ep_dequeue (epdata->ep, priv->req);
	else
		value = -EINVAL;
	// spin_unlock(&epdata->dev->lock);
	local_irq_enable();

	aio_put_req(iocb);
	return value;
}

And adding extra aio_put_req() to the cancel method will not fix failing 
kick_iocb() which is another problem and this patch is supposed to address it.

> I'd rather not muddy these waters even further.  Unless there is a
> compelling reason why the patch author cannot do an extra put in his
> cancel method, I'd say that is the best way forward.
> 

This patch is not about reference counting - it's about kick_iocb() failing 
due to NULL values of iocb->ki_run_list.next and iocb->ki_run_list.prev which 
are left after aio_run_iocb().

> Cheers,
> Jeff
> 

-- 
Regards, Sergey Temerkhanov,
Cifronic ZAO

Re: [PATCH][RFC] AIO: always reinitialize iocb->ki_run_list at the end of aio_run_iocb()

[Jeff Moyer]

Sergey Temerkhanov <temerkhanov@yandex.ru> writes:

> On Wednesday 26 May 2010 23:38:35 Jeff Moyer wrote:
> ...
>> I can vaguely recall discussion surrounding the reference counting of
>> cancel methods, but I have no idea what the actual contents of those
>> discussions were.  Sorry, my memory has failed me.  Either Zach or
>> Suparna might remember better.
>> 
>> Sergey, the cancellation path, unfortunately, is not well exercised as
>> I'm sure you are aware.  As you pointed out, the only implementation of
>> a cancel method is the usb gadget interface.  Now, given that they've
>> worked fine with the extra put in their cancel method, I'm not sure why
>> you can't do the same. 
> Well, in fact, they have only one aio_put_req() in their cancel method. This 
> is the code from 2.6.34:

I was referring to the aio_put_req done deeper in the call chain by the
completion methods for the usb gadgetfs request.

> And adding extra aio_put_req() to the cancel method will not fix failing 
> kick_iocb() which is another problem and this patch is supposed to address it.

I guess I'm confused.  You wrote the following:

> I've written the driver code which implements a zero-copy DMA char device. It 
> has aio_read() and aio_write() methods which return -EIOCBQUEUED after the 
> successful preparation of the buffers described by kiocb and posting it to the 
> descriptor chain. When the descriptors are processed, the DMA engine raises 
> the interrupt and the cleanup work is done in the handler, including 
> aio_complete() for the completed kiocbs.
>
> This works fine, however, there is a problem with canceling the queued 
> requests, espesially on io_destroy() syscall. Since there is no simple way to 
> remove single kiocb from the descriptor chain, I'm removing all of them from 
> the queue using aio_complete() or aio_put_req() in the ki_cancel() callback 
> routine of my driver. The main problem is the reference counting in 
> aio_cancel_all():
>
> 		if (cancel) {
> 			iocb->ki_users++;
> 			spin_unlock_irq(&ctx->ctx_lock);
> 			cancel(iocb, &res);
> 			spin_lock_irq(&ctx->ctx_lock);
> 		}
>
> Here the iocb->ki_users gets incremented which already has the value 1 at this 
> point (after the io_submit_one() completion) and it's never released (). So I 
> have to call aio_put_req() twice for the given kiocb (this seems to be the 
> hack to me) or I'll end up with the unkillable process stuck in 
> wait_for_all_aios() at the io_schedule(). I've posted the patches where I've 
> added aio_put_req() but I think it needs more testing. 

OK, you tried two aio_put_req() calls, and it worked, but you thought
maybe it wasn't the right approach.  So:

> So, I've tried another approach (hack) - requeue the kiocb with
> kick_iocb() before calling aio_put_req() in the ki_cancel() callback
> (that's because aio_run_iocb() takes some special actions for the
> canceled kiocbs). And I've found out that kick_iocb() fails because
> aio_run_iocb() does this:
> 	iocb->ki_run_list.next = iocb->ki_run_list.prev = NULL;
> and only reinitializes iocb->ki_run_list when iocb->ki_retry() returns 
> -EIOCBRETRY but kick_iocb() is exported and looks like intended for usage 
> (though not recommended).

You implemented this other hack, and ran into troubles, so you're
modifying the aio core to fix it.  Am I wrong in concluding that if you
keep your first solution above, you no longer need this second?

You may also find the following an interesting read:

http://permalink.gmane.org/gmane.linux.kernel.aio.general/2571

Cheers,
Jeff

Re: [PATCH][RFC] AIO: always reinitialize iocb->ki_run_list at the end of aio_run_iocb()

[Sergey Temerkhanov]

On Wednesday 02 June 2010 01:14:48 Jeff Moyer wrote:
> Sergey Temerkhanov <temerkhanov@yandex.ru> writes:
> > On Wednesday 26 May 2010 23:38:35 Jeff Moyer wrote:
> > ...
> OK, you tried two aio_put_req() calls, and it worked, but you thought
> 
> maybe it wasn't the right approach.  So:
> > So, I've tried another approach (hack) - requeue the kiocb with
> > kick_iocb() before calling aio_put_req() in the ki_cancel() callback
> > (that's because aio_run_iocb() takes some special actions for the
> > canceled kiocbs). And I've found out that kick_iocb() fails because
> > aio_run_iocb() does this:
> > 	iocb->ki_run_list.next = iocb->ki_run_list.prev = NULL;
> > and only reinitializes iocb->ki_run_list when iocb->ki_retry() returns
> > -EIOCBRETRY but kick_iocb() is exported and looks like intended for usage
> > (though not recommended).
> 
> You implemented this other hack, and ran into troubles, so you're
> modifying the aio core to fix it.  Am I wrong in concluding that if you
> keep your first solution above, you no longer need this second?

Well, let's forget the reference counting for now since failing kick_iocb() is 
generally not related to it. I've just described how I've run into it 
- I could run into it somehow else.

The problem with kick_iocb() is exactly as this: If retry() method 
returns -EIOCBQUEUED to aio_run_iocb() then any subsequent call to 
kick_iocb() results in failure. Is it expected behavior?

> 
> You may also find the following an interesting read:
> 
> http://permalink.gmane.org/gmane.linux.kernel.aio.general/2571
> 
> Cheers,
> Jeff
> 


-- 
Regards, Sergey Temerkhanov,
Cifronic ZAO

Re: [PATCH][RFC] AIO: always reinitialize iocb->ki_run_list at the end of aio_run_iocb()

[Jeff Moyer]

Sergey Temerkhanov <temerkhanov@cifronik.ru> writes:

> The problem with kick_iocb() is exactly as this: If retry() method 
> returns -EIOCBQUEUED to aio_run_iocb() then any subsequent call to 
> kick_iocb() results in failure. Is it expected behavior?

 * If ki_retry returns -EIOCBQUEUED it has made a promise that aio_complete()
 * will be called on the kiocb pointer in the future.  The AIO core will
 * not ask the method again -- ki_retry must ensure forward progress.

Does that answer your question?

Cheers,
Jeff