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

Mark Geisert mark@maxrnd.com
Wed Jul 18 05:26:00 GMT 2018


Corinna Vinschen wrote:
> Hi Mark,
>
> this looks good.  Inline comments as usual.

Thank you; OK.

> On Jul 15 01:20, Mark Geisert wrote:
[...]
>> +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?

Brain hiccup on my part.  I was thinking read-only simple access doesn't need to 
be locked.  That's wrong and fixed now.

>> +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 :)

I've now added commentary above the pthread_create() call.  It reads:
   /* A "vaquita" thread is a temporary pthread created to deliver a signal to
    * the application.  We don't wait around for the thread to return from the
    * app.  There's some symbolism here of sending a little creature off to tell
    * the app something important.  If all the vaquitas end up wiped out in the
    * wild, a distinct near-term possibility, at least this code remembers them.
    */

If all this vaquita stuff is deemed too precious for industrial-grade software I 
can recast this code in more professional terms and wouldn't mind doing 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 :)

Thanks for the background; I missed the 2nd arg being passed by reference.  I've 
left the code as-is but have now added a little commentary to describe what's 
going on.  Re-writing the fhandler read method is left for the future...

>
>> +#ifdef _POSIX_SYNCHRONIZED_IO
>
> We really don't need this ifdef, not even in the header.  The macro is
> certainly defined.

All occurrences have now been deleted.

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

It's totally OK; I'm still fine tuning the replacement code for readability and 
ease of coding.

..mark



More information about the Cygwin-patches mailing list