[PATCH v3 1/3] POSIX Asynchronous I/O support: aio files

[Corinna Vinschen]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-patches/attachments/20180716/1d83d22a/attachment.sig>