--AbQceqfdZEv+FvjW
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 30, 2007 at 11:13:18PM +0100, Andrew Doran wrote:
> Hi,
>=20
> > Log Message:
> > Import of POSIX Asynchronous I/O.
> > Seems to be quite stable. Some work still left to do.
>=20
> Cool! I have some more comments & suggestions now that this is in the tre=
e.
>=20
> o It belongs in sys_aio.c for a couple of reasons. The file implements on=
ly
>   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.

Agreed.

> 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 th=
at
>   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 perha=
ps
>   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:
>=20
> 	mutex_enter(&p->p_mutex);
> 	if ((p->p_flag & PK_AIOINIT) !=3D 0 || p->p_aio !=3D NULL) {
> 		/* Already initializing or initialized. */
> 		while ((p->p_flag & PK_AIOINIT) !=3D 0)
> 			cv_wait(&p->p_mutex, &p->p_somecv);
> 		if (p->p_aio !=3D NULL) {
> 			mutex_exit(&p->p_mutex);
> 			return;
> 		} else {
> 			/*
> 			 * Another thread's initialization attempt
> 			 * may have failed: retry.
> 			 */
> 		}
> 	}

Layered file systems deal with this. You grab the mutex & look to see if=20
it's been inited. If not, you drop the mutex, do the allocation, grab the=
=20
mutex, & see if it's still uninitialized. If uninitialized, store your=20
allocation & proceed. If it's been allocated by someone else, error out=20
and release what you allocated.

> 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:
>=20
> 	/* No longer initializing. */
> 	mutex_enter(&p->p_mutex);
> 	p->p_aio =3D aio;
> 	p->p_flag &=3D ~PK_AIOINIT;
> 	cv_broadcast(&p->p_somecv);
> 	mutex_exit(&p->p_mutex);

How much is involved in cleaning up a failed initialization? Another=20
option is to fill in the aio structure before storing it in the process=20
structure.

Take care,

Bill

--AbQceqfdZEv+FvjW
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (NetBSD)

iD8DBQFGNmzEWz+3JHUci9cRAg8wAKCMx6wKQuiHQb7WchBwdlTBLweuEACfTt7b
kDdcT2k+QR33pbDUk4ZszYI=
=9sPY
-----END PGP SIGNATURE-----

--AbQceqfdZEv+FvjW--