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.
On Wed, 28 Apr 2010 02:51:43 +0400
Sergey Temerkhanov <[email protected]> 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.
On Wednesday 28 April 2010 22:31:49 Andrew Morton wrote:
> On Wed, 28 Apr 2010 02:51:43 +0400
>
> Sergey Temerkhanov <[email protected]> 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 <[email protected]>
Regards, Sergey Temerkhanov,
Cifronic ZAO
Sergey Temerkhanov <[email protected]> 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
On Wednesday 02 June 2010 01:14:48 Jeff Moyer wrote:
> Sergey Temerkhanov <[email protected]> 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
Sergey Temerkhanov <[email protected]> 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