[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