On Tue, May 01, 2007 at 03:36:30PM +0300, Mindaugas R. wrote:

> People... where you have been all that week?...
> 
> Andrew Doran <ad@netbsd.org> wrote:
> > o It belongs in sys_aio.c for a couple of reasons. The file implements only
> >   syscalls and direct support for them, and it's not a VFS component as
> >   such. It should be possible to do AIO on other types of file descriptor.
> So you want to rename vfs_aio.c to sys_aio.c, after commit?... :(

Yup.

> OK, but the same is valid for vfs_bio.c, and probably various other files.

The code in vfs_bio.c is fairly intertwined with the way filesystems and
vnodes work.. For other files sure, but it doesn't prevent us from doing
the correct thing in this case.

> > o I'm still not fond of copying out the state to userspace only for the
> >   kernel to copy it back in later when asked for it. I'm not familiar with
> >   the ins and outs of the POSIX AIO stuff, but I'd prefer to see some kind
> >   of job control block stick around in the kernel until userspace calls in
> >   to collect it. Would that be possible?
> Understood. There are no, of course, POSIX requirements for that. But
> avoiding copyin() would cost the locking of aio_mtx and searching in the
> queue.
> Keeping in mind that people also write such code:
> 	while (aio_error (aiocbp) == EINPROGRESS);
> .. I would prefer the current way.

Well it's probably fine as it is. It's not like we can't change it later if
there is a good reason to.

> > o Holding p->p_mutex across mutex_init and pool_init is dicey since they can
> >   sleep in order to allocate memory.  There are number of other things that
> >   depend on p_mutex, so if an LWP sleeps here it could tie other LWPs up in
> >   knots waiting. What I suggest to fix that is a flag in p_flag and perhaps
> >   another condvar that says "AIO is being initialized". Other threads that
> >   end up in aio_init would wait for that flag to clear, then return when it
> >   has cleared:
> > <...>
> > o Testing for p->p_aio to be non-null appears to be unsafe as it can
> >   currently be non-null while AIO is not fully initialized. To fix that
> >   the worker thread could also check for the "AIO is initializing" flag in
> >   p_flag, and wait until it clears. Then, we could set p_aio as the last
> >   action in aio_init, like this:
> > <...>
> Previously you have suggested to make aio_mtx per proc structure, and
> initialize it when creating a process. Here is a recheck:
> 	/* Recheck if we are really first */
> 	mutex_enter(&p->p_mutex);
> 	if (p->p_aio) {
> 		mutex_exit(&p->p_mutex);
> 		kmem_free(aio, sizeof(struct aioproc));
> 		return 0;
> 	}
> 	...

I think that Bill's suggestion of setting everything up, then checking to
see if someone else beat us to it before returning is the best way to handle
it. Unfortunatley that requires an lwp_exit() that works on LWPs other than
curlwp. I have a patch to do that lurking somewhere, I'll see about digging
it up.

Andrew