Hi Mark, this looks good. Inline comments as usual.
On Jul 15 01:20, Mark Geisert wrote:
> + * (2) no more than AIO_MAX inline AIOs will be in progress at same time.
> + * In all other cases queued AIOs will be used.
> + *
> + * An inline AIO is performed by the calling app's thread as a pread|pwrite
> on
> + * a shadow fd that permits Windows asynchronous i/o, with event notification
> + * on completion. Event arrival causes AIO context for the fd to be updated.
> + *
> + * A queued AIO is performed in a similar manner, but by an AIO worker thread
> + * rather than the calling app's thread. The queued flavor can also operate
> + * on sockets, pipes, non-binary files, mandatory-locked files, and files
> + * that don't support pread|pwrite. Generally all these cases are handled as
> + * synchronous read|write operations, but still don't delay the app because
> + * they're taken care of by AIO worker threads.
> + */
> +
> +/* These variables support inline AIO operations */
> +static NO_COPY HANDLE evt_handles[AIO_MAX];
> +static NO_COPY struct aiocb *evt_aiocbs[AIO_MAX];
> +static NO_COPY CRITICAL_SECTION evt_locks[AIO_MAX]; /* per-slot locks */
> +static NO_COPY CRITICAL_SECTION slotcrit; /* lock for slot variables in
> toto */
> +
> +/* These variables support queued AIO operations */
> +static NO_COPY sem_t worksem; /* tells whether AIOs are queued
> */
> +static NO_COPY CRITICAL_SECTION workcrit; /* lock for AIO work queue
> */
> +TAILQ_HEAD(queue, aiocb) worklist = TAILQ_HEAD_INITIALIZER(worklist);
> +
> +static int
> +aiochkslot (struct aiocb *aio)
> +{
> + /* Sanity check.. make sure this AIO is not already busy */
> + for (int slot = 0; slot < AIO_MAX; ++slot)
> + if (evt_aiocbs[slot] == aio)
> + {
> + debug_printf ("aio %p is already busy in slot %d", aio, slot);
> + return slot;
> + }
> +
> + return -1;
> +}
Shouldn't this check be under lock as well? Or I am missing something.
Why is the lock not necessary here?
> +aionotify_on_pthread (struct sigevent *evp)
> +{
> + pthread_attr_t *attr;
> + pthread_attr_t default_attr;
> + int rc;
> + pthread_t vaquita; /* == "little porpoise", endangered */
> +
> + if (evp->sigev_notify_attributes)
> + attr = evp->sigev_notify_attributes;
> + else
> + {
> + pthread_attr_init (attr = &default_attr);
> + pthread_attr_setdetachstate (attr, PTHREAD_CREATE_DETACHED);
> + }
> +
> + rc = pthread_create (&vaquita, attr,
> + (void * (*) (void *)) evp->sigev_notify_function,
> + evp->sigev_value.sival_ptr);
> +
> + /* The following error is not expected. If seen often, develop a recovery.
> */
> + if (rc)
> + debug_printf ("aio vaquita thread creation failed, %E");
I like the name, but what's the background for naming a thread like this?
Just curious. A bit of comment might help to keep it in mind, too :)
> + /* If we thought we had a slot for this queued async AIO, but lost out
> */
> + if (aio->aio_errno == ENOBUFS)
> + {
> + aio->aio_errno = EINPROGRESS;
> + aioqueue (aio); /* Re-queue the AIO */
> +
> + /* Another option would be to fail the AIO with error EAGAIN, but
> + * experience with iozone showed apps might not expect to see a
> + * deferred EAGAIN. I.e. they should expect EAGAIN on their call
> to
> + * aio_read() or aio_write() but probably not expect to see EAGAIN
> + * on an aio_error() query after they'd previously seen EINPROGRESS
> + * on the initial AIO call.
> + */
> + continue;
> + }
Got it.
> + /* Do the requested AIO operation manually, synchronously */
> + switch (aio->aio_lio_opcode)
> + {
> + case LIO_READ:
> + /*XXX Hmm, no direct result? This OK? */
Unfortunately the fhandler read method has been written this way more
than 20 years ago. I have no idea why and it's ugly as hell.
But there is a result: The second parameter is set to the number of bytes
read or -1 on error, in which case errno is set.
Feel free to rewrite the fhandler read method to look more sane :)
> +#ifdef _POSIX_SYNCHRONIZED_IO
We really don't need this ifdef, not even in the header. The macro is
certainly defined.
> diff --git a/winsup/cygwin/include/aio.h b/winsup/cygwin/include/aio.h
> new file mode 100644
> index 000000000..34ff29969
> --- /dev/null
> +++ b/winsup/cygwin/include/aio.h
> @@ -0,0 +1,82 @@
> +/* aio.h: Support for Posix asynchronous i/o routines.
> +
> +This file is part of Cygwin.
> +
> +This software is a copyrighted work licensed under the terms of the
> +Cygwin license. Please consult the file "CYGWIN_LICENSE" for
> +details. */
> +
> +#ifndef _AIO_H
> +#define _AIO_H
> +
> +#include <sys/features.h>
> +#include <sys/queue.h>
> +#include <sys/signal.h>
> +#include <sys/types.h>
> +#include <limits.h> // for AIO_LISTIO_MAX, AIO_MAX, and AIO_PRIO_DELTA_MAX
> +
> +#ifndef __INSIDE_CYGWIN__
> +#include <w32api/winternl.h> // for HANDLE and IO_STATUS_BLOCK
> +#endif
Hmm. Did I miss this last time? Looks like it.
> +/* struct aiocb is defined by Posix */
> +struct aiocb {
> + /* these elements of aiocb are Cygwin-specific */
> + TAILQ_ENTRY(aiocb) aio_chain;
> + struct liocb *aio_liocb;
> + HANDLE aio_win_event;
> + IO_STATUS_BLOCK aio_win_iosb;
> + ssize_t aio_rbytes;
> + int aio_errno;
> + /* the remaining elements of aiocb are defined by Posix */
> + int aio_lio_opcode;
> + int aio_reqprio; /* Not yet implemented; must be zero */
> + int aio_fildes;
> + volatile void *aio_buf;
> + size_t aio_nbytes;
> + off_t aio_offset;
> + struct sigevent aio_sigevent;
> +};
Can you please change this so it doesn't require to include a
windows header? You could use void * instead of HANDLE and define
your own __io_t (or whatever) as a struct of void * and uintptr_t
values and only cast them to the Windows types inside of Cygwin.
That ok?
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
signature.asc
Description: PGP signature
