Re: lio_listio fixes and adding kqueue notification patch

Doug Ambrisko wrote this message on Thu, Oct 21, 2004 at 16:48 -0700:
> 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.

np...

> | 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.

Ok, we should mark the kevent as EV_ONESHOT, then once the event has
finished, we could simply detached the knote, and not need to clean it
up anymore...

> | 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 

ahh, ok. yeh, I see that now...

> 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

actually, we need to cut out the second paragraph.. hmmm. this is
funny, EV_FLAG1 is not restricted to the kernel, which means that the
userland can register an AIO request...

> 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.

yeh, definately need to fix this...

> | [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.

sure...  we'll find a solution... do you have some good test programs?

-- 
  John-Mark Gurney				Voice: +1 415 225 5579

     "All that I will do, has been done, All that I have, has not."