Re: lio_listio fixes and adding kqueue notification patch

John-Mark Gurney writes:
| Doug Ambrisko wrote this message on Wed, Oct 20, 2004 at 12:10 -0700:
| > Here's a patch to fix some lio_listio bugs and add kqueue support to it.
| > BTW kqueue & aio_read/write is broken and that seems to have happened
| > during the locking changes.  I could use some more testers for the
| > signal causes.
| > 
| > http://www.ambrisko.com/doug/listio_kqueue/listio_kqueue.patch
| > 
| > The summary is:
| > kqueue changes:
| >       -	kqueue locking removed/broke knote_remove (transformed into
| > 	knlist_clear??) which no longer deletes and clean up the
| > 	the knotes so they are left hanging around and can break the
| > 	next transfer since they are already triggered etc.
| >       -	add knlist_delete to restore knote_remove functionality to
| > 	totally take it of the system.  When you do an aio_return
| > 	(which implies aio_free_entry) the kevent can never trigger
| > 	again since the context is lost.
| 
| why did you copy so much of knlist_clear into knlist_delete?  why not
| expand knlist_clear to optionally do the delete instead of duplicating
| the code?  Also, why do you duplicate all the logic in knote_drop into
| knlist_delete?  why not make a knote_drop_locked function, and call it
| from knlist_delete?

Since I'm not an expert on this stuff and I didn't want to break
the existing code ... such as happened when the locking was done.
When I initially did some change I ran into locking issues that were not
obvious to figure out.  I'm willing to adjust that as you suggest.  Didn't 
want to spend a lot of time it and deal with merge conflicts.
This is way I asked for a review to work toward a proper solution.
 
| I'll send you a kqueue manpage that I've written so far for your
| review...
| 
| I don't like removing knotes completely w/o it being returned to the
| userland unless there is good reason (like the user did a close on an
| fd and expects the knote to go away)...  some people (like me) use knote's
| in the kernel to keep track of resources...  so, will it be documented
| that this is what happens?

When an aio operation is complete it is dead and it can't happen
again.  The aio_return is acknowledgement that the transaction is 
complete and can't happen again like a close (the only thing different
from a close is that other I/O can still happen on the fd.  If you leave it
around then the next aio op gets the prior knote done when it hasn't
done anything.  Bad things happen.  This is documented in the aio 
usage stuff.

| also, why arey ou adding an splbio call?  spl calls in 5.x and 6.x are
| if def'd to nothing.. it'd be better to add an XXX comment about requiring
| locking...  or GIANT_REQUIRED;

Consistancy for back-porting to 4.X ... it already has them.  Might as
well make it correct in the one minor case that I changed it.  It is
a minor point.  Future work would be to make it giant free like Alfred
has started.  I brought this code up to 5.X via diff and patch
since I did this implementation under 4.X.  So that no big thing
to change if absolutely needed.

| >       -	hack, remove the mtx_assert in knlist_empty so it can be used
| > 	to see if there are kqueue's on an aio task.
| 
| I don't like this..  The problem I have with this is that between the
| time that knlist_empty returns, there could be a knote added...  so any
| decision based upon that check is stale?  If you know you aren't going
| to be adding anymore knlist_clear (and by the copying knlist_delete) is
| safe to call w/o having to check knlist_empty..  They aren't going to
| be doing much more work that knlist_empty isn't going to do...  (and
| prevents you having to remove the mtx_assert in knlist_empty)..

How are the knotes added since the aio call makes them when
you submit an aio I/O request and only lives until acknowledged by 
aio_return.  So aio creates, registers and clears the kevent.  The user 
then listens to them.  If there are no knotes then you are going to 
use the signal method of delivery.  Mostly it is a panic saver of 
which I hit a few.  Strictly it isn't needed but it doesn't hurt.  
kqueue & aio is special.  Have you read the EVFILT_AIO part of 
man kqueue?  It has to be registered on the aio operation or you
might not get it registered fast enough before it returns.  If you
happen to register a kevent on the aio op. outside the aio commands
you might never get notified since the check is only done on I/O
completion which may have already occured.  So this isn't a problem.

| [aio stuff deleted]
| 
| can't really comment on the aio stuff since I don't know it that
| well..

Which is why you broke it and it doesn't work now :-(  The lio_listio
is pretty busted as is and doesn't work reliably regardless of kevent.

I hope we can work to a solution that is stable and works well with aio.
The guts are here it more of an issue of how to best integrate it.

Doug A.