Re: [PATCH] aio_run_iocb should always retry

> From: Zach Brown <[email protected]>
>
> > I've been using gadgetfs (the USB slave filesystem) as an example for
> > how to do in-kernel AIO.
>
> Oh dear.  It'd probably be better if you asked linux-aio and we  
> updated the docs as you found confusing parts.  Hell, just mail me  
> directly.  Anything but having to read that thing and suffer through  
> a near-total lack of documenting comments.

Yes, the AIO code certainly had a *lot* of confusion on its
cancelation code paths.  And I always thought its terminology
was kind of odd ... e.g. "retry" was at core just a "continue
this transfer", but it accumulated all sorts of odd peripheral
semantics because of that name.  (A "retry" does the same thing
over again.  That's virtually never appropriate for file I/O
operations...)


> > The problem comes when someone calls sys_io_cancel() on a read.  If  
> > the
> > retry function has started before this point, everything is fine.
> > However, if sys_io_cancel() runs before the retry function,  
> > aio_run_iocb
> > will notice the iocb is cancelled and call aio_complete instead.  The
> > retry function never runs, and the data structures are never freed.

ISTR there was once a non-obvious way to make that work, several
years ago, which seems to have subsequently been broken.  (Presumably
it was the usual one-bug-hiding-another-hiding-another thing, where
fixing one thing breaks something else...)

The locking seemed goofy too ... in gadgetfs, ep_eio_cancel() could
not grab the lock it should have.  (And four years later, I certainly
don't recall details about why not!)


> So, as written today, fs/aio.c expects ki_cancel to be *very*  
> carefully written.  It doesn't serialize ki_cancel calls against,  
> well, anything of consequence.

ISTR noticing that not-so-little problem...


>	It can be called concurrently.  It  
> can race with ki_retry.  It can be called *after* a final ki_retry  
> has completed but before the iocb has been removed from the iocb  
> list.  (A malicious app can probably get it called the *moment* you  
> set ki_cancel, terrifying.)
>
> Along that theme, it's expected to very carefully serialize with  
> ki_retry.  It requires that it can serialize with the mechanism that  
> ki_retry will rely on to see that aio_complete() or kick_iocb() are  
> called.  That requirement, seemingly, requires knowing that being  
> called in ki_cancel implies that a previous kick_iocb() call might  
> now only drop the iocb ref and won't call ki_retry.
>
> So, to the case you're worried about.  kick_iocb() has been called  
> but the bits of epdata haven't been freed yet.  To work with the  
> current fs/aio.c code ep_aio_cancel() would have to notice that  
> kick_iocb() has been called, but the stuff hasn't been freed, so it  
> should free it then.  It might require more locking around that usb  
> req queue, who knows.  (commented out spin locks are scary!)

All I remember about that commenting-out was that although the
data structures being accessed *should* have been protected by
that lock -- basic "what does it protect" policy --  things didn't
work right if it did so.  ISTR that under stress loads it could
sometimes get called when it already held the lock, or somesuch.
Self deadlock and all that; as you said, it doesn't serialize...

Regardless, on uniprocessors -- typical for systems with USB
peripheral hardware -- it seemed sufficient to disable IRQs.  I
didn't like that "solution" either, but I got the feeling back
then that some people just wanted non-block AIO to vanish, as
if it wasn't one of the original motivations for such operating
system mechanisms.  ;)


> > Jamey and I believe there is no way to know in the cancel function if
> > the retry function will be called.
>
> I suspect that you could serialize around whatever queue it is that  
> usb_ep_dequeue() is operating on.  I'm not saying that you *should*,  
> just that it'd be one way to address the imbalance between what fs/ 
> aio.c expects and what ep_aio_cancel() is doing.

The dequeue() operation works at a level below this driver; it has
no access to that queue or its locks.  Also, note that there's no
requirement that dequeue() be synchronous.  The hardware may need
to abort a DMA transfer ... which can take a long time when there's
a hardware DMA queue to stop, and all too often hits lots of chip
errata related to synchronizing the DMA engine with a given fifo.


> > Therefore, kick_iocb() should always call the retry function.  This  
> > patch fixes that bug.
>
> In any case, I don't think this is the right solution.  gadgetfs is  
> pretty unusual in using one ki_retry to submit and then switching to  
> another to copy the result to userspace and free.  The intent of  
> ki_retry is to, well, retry.

What are you talking about?  There's only one retry function!!

And if there's a better way to ensure that the right MM context
is available for copying the data into userspace, I sure didn't
see it four years ago.  In fact, that seemed to be the only sane
place to put such logic...



> In particular, if we just fix this freeing problem by calling retry  
> after cancellation then we still have the problem where a racing  
> ep_aio_cancel() can try and reference freed memory if its called  
> after ep_aio_read_retry() returns but before fs/aio.c takes the iocb  
> out of the list.
>
> No, I think we should step back and fix the fundamental serialization  
> problems with sys_io_cancel().  Right now it requires each ki_cancel  
> implementation to take on way too much complexity.  How about we make  
> the following rules:
>
> - ki_cancel will not be called while ki_retry is being called
>
> - ki_cancel will not be called after ki_retry returns codes which  
> complete the iocb
>
> - ki_cancel will not be called until ki_retry has returned at least once

That one won't work.  The reason for cancelation will normally be
that the I/O has blocked -- so retry() will *never* be called!

Plus, when returning after zero byte I/O (protocol-significant in USB),
retry() may never be called... right now that's got the most complete
fault reporting available too, including both a status code and a
count of byte.  Way back when, the retry() path didn't allow that.
(Ideally the interface glitches which imposed that constraint are now
gone, and it's just waiting for someone to update gadgetfs...)


> - ki_cancel after kick_iocb() will not stop ki_retry from being called
>
> - ki_cancel will not be called after aio_complete() has been called
>
> The intention is to only call ki_cancel when we're in that giant idle  
> time after when ki_retry has returned either EIOCBQUEUED or  
> EIOCBRETRY but before either aio_complete() or kick_iocb() have been  
> called.  In the cases where sys_io_cancel() finds that it shouldn't  
> call ki_cancel it could return EAGAIN.

It should probably be OK to call ki_cancel() at any point before
aio_complete() is called, after the kiocb is activated.  If it's
called outside of that window, the answer should be deterministic.
A kiocb should have a very simple lifecycle...


> Serializing ki_retry and ki_cancel calls will be easy because they're  
> called from process context from fs/aio.c.  Serializing with  
> aio_complete() will be harder as its called from interrupts from  
> subsystems.  We might just add a call to let subsystem aio_complete()  
> callers or ki_cancel implementations negotiate who should free  
> resources.  Or leave that serialization up to the subsystems.  I'm  
> not sure.

Hmm, well all that sounds like it ought to help something, but it's
been a long time since I looked at AIO, and I understand some of the
way it works has changed.  So I won't try to poke too hard at that...

Re releasing resources, I'd think that whoever allocates them should
free them... possibly there should be a hook to delegate that stuff.
So for example, such a hook in gadgetfs could release the usb_request
and its associated buffer.

- Dave


> If this sounds OK I could roll some patches.  Please tell me that you  
> guys have some tests which are failing with the current code so we  
> can verify our work.
>
> - z
>
>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel