# Attempt to fix pread() threading issue on Cygwin and MinGW 2 messages from 2012-06-26 to 2012-06-26. Participants: Ramsay Jones, Erik Faye-Lund. Thread: https://gitlist.dev/t/30910 ## Ramsay Jones, 2012-06-26 19:00 Subject: Attempt to fix pread() threading issue on Cygwin and MinGW Message-ID: <4FEA06DE.6080101@ramsay1.demon.co.uk> URL: https://gitlist.dev/e/4FEA06DE.6080101%40ramsay1.demon.co.uk ``` Hi Johannes, I had an "fix pread() on MinGW" item low on my todo list. As we have seen recently, Cygwin has the same problem. So, I decided to try and fix it up last night, since I had an idea that I thought would work. Several years ago, I read somewhere (a Microsoft Press publication of some sort) that, even when using *synchonous* I/O, you could pass an OVERLAPPED structure to ReadFile() and have it use the file offset in that structure, rather than the implicit stream pointer. So, I modified my "hacked up test program" from the other day to try it out; the diff looks like: -- >8 -- diff --git a/test-pread.c b/test-pread.c index 61280cb..8a2b2ff 100644 --- a/test-pread.c +++ b/test-pread.c @@ -1,5 +1,6 @@ #include "git-compat-util.h" #include "thread-utils.h" +#include #define DATA_FILE "junk.data" #define MAX_DATA 256 * 1024 @@ -16,6 +17,31 @@ struct thread_data { static struct thread_data t[NUM_THREADS+1]; +ssize_t pread_(int fd, void *buf, size_t count, off_t offset) +{ + OVERLAPPED o = {0}; + HANDLE fh = (HANDLE)_get_osfhandle(fd); + uint64_t off = offset; + DWORD bytes; + BOOL ret; + + if (fh == INVALID_HANDLE_VALUE) { + errno = EBADF; + return -1; + } + + o.Offset = off & 0xffffffff; + o.OffsetHigh = (off >> 32) & 0xffffffff; + + ret = ReadFile(fh, buf, (DWORD)count, &bytes, &o); + if (!ret) { + errno = EIO; + return -1; + } + + return (ssize_t)bytes; +} + int create_data_file(void) { int i, fd = open(DATA_FILE, O_CREAT | O_TRUNC | O_WRONLY, 0600); @@ -55,7 +81,7 @@ void *pread_thread(void *data) ssize_t sz; d->n = d->n * 1103515245 + 12345; j = d->n % MAX_DATA; - sz = pread(d->fd, &rd, sizeof(int), j * sizeof(int)); + sz = pread_(d->fd, &rd, sizeof(int), j * sizeof(int)); if (sz < 0 || rd != j) d->fails++; d->cnt++; -- 8< -- The result of running the updated program looks like: ramsay $ gcc -I. -o test-pread test-pread.c ramsay $ ./test-pread.exe 0: trials 524288, failed 262139 1: trials 500000, failed 0 2: trials 500000, failed 0 3: trials 500000, failed 0 ramsay $ So, the results are a little disappointing. :( Although ReadFile() is indeed using the OVERLAPPED structure to specify the read position, it is still updating the implicit stream position. So, this implementation does not faithfully reproduce the full semantics of pread(), since it updates the stream position (affecting other I/O calls which rely on the implicit file position. eg read()). [Note: this was tested on Windows XP SP3. Maybe Vista/Win7/Win8 would behave differently?] This implementation should be sufficient to fix the immediate problem with the threaded index-pack, but I'm not sure it would be a good idea to provide an *almost* compliant pread(). I've attached the full test program below, just for completeness. ATB, Ramsay Jones -- >8 -- #include "git-compat-util.h" #include "thread-utils.h" #include #define DATA_FILE "junk.data" #define MAX_DATA 256 * 1024 #define NUM_THREADS 3 #define TRIALS 500000 struct thread_data { pthread_t t; int fd; int cnt; int fails; unsigned long n; }; static struct thread_data t[NUM_THREADS+1]; ssize_t pread_(int fd, void *buf, size_t count, off_t offset) { OVERLAPPED o = {0}; HANDLE fh = (HANDLE)_get_osfhandle(fd); uint64_t off = offset; DWORD bytes; BOOL ret; if (fh == INVALID_HANDLE_VALUE) { errno = EBADF; return -1; } o.Offset = off & 0xffffffff; o.OffsetHigh = (off >> 32) & 0xffffffff; ret = ReadFile(fh, buf, (DWORD)count, &bytes, &o); if (!ret) { errno = EIO; return -1; } return (ssize_t)bytes; } int create_data_file(void) { int i, fd = open(DATA_FILE, O_CREAT | O_TRUNC | O_WRONLY, 0600); if (fd < 0) return -1; for (i = 0; i < MAX_DATA; i++) if (write(fd, &i, sizeof(int)) < 0) { close(fd); unlink(DATA_FILE); return -1; } close(fd); return 0; } void *read_thread(void *data) { struct thread_data *d = (struct thread_data *)data; int i, j, rd; for (i = 0; i < TRIALS; i += MAX_DATA) { for (j = 0; j < MAX_DATA; j++) { ssize_t sz = read(d->fd, &rd, sizeof(int)); if (sz < 0 || rd != j) d->fails++; d->cnt++; } lseek(d->fd, 0, SEEK_SET); } return NULL; } void *pread_thread(void *data) { struct thread_data *d = (struct thread_data *)data; int i, j, rd; for (i = 0; i < TRIALS; i++) { ssize_t sz; d->n = d->n * 1103515245 + 12345; j = d->n % MAX_DATA; sz = pread_(d->fd, &rd, sizeof(int), j * sizeof(int)); if (sz < 0 || rd != j) d->fails++; d->cnt++; } return NULL; } int main(int argc, char *argv[]) { int fd, i; if (create_data_file() < 0) { printf("can't create data file\n"); return 1; } if ((fd = open(DATA_FILE, O_RDONLY)) < 0) { printf("can't open data file\n"); unlink(DATA_FILE); return 1; } for (i = 0; i < NUM_THREADS+1; i++) { int ret; t[i].fd = fd; t[i].cnt = 0; t[i].fails = 0; t[i].n = i * 16381; ret = pthread_create(&t[i].t, NULL, (i == 0) ? read_thread : pread_thread, &t[i]); if (ret) { printf("can't create thread %d (%s)\n", i, strerror(ret)); unlink(DATA_FILE); return 1; } } for (i = 0; i < NUM_THREADS+1; i++) pthread_join(t[i].t, NULL); close(fd); for (i = 0; i < NUM_THREADS+1; i++) printf("%2d: trials %d, failed %d\n", i, t[i].cnt, t[i].fails); unlink(DATA_FILE); return 0; } ``` ## Erik Faye-Lund, 2012-06-26 19:46 Subject: Re: Attempt to fix pread() threading issue on Cygwin and MinGW Message-ID: URL: https://gitlist.dev/e/CABPQNSZWLvYGH6sFkWa588%2BfPJXV3c5ViKsEyMABViN74ghaLg%40mail.gmail.com In-Reply-To: <4FEA06DE.6080101@ramsay1.demon.co.uk> ``` On Tue, Jun 26, 2012 at 9:00 PM, Ramsay Jones wrote: > Hi Johannes, > > I had an "fix pread() on MinGW" item low on my todo list. As we have > seen recently, Cygwin has the same problem. So, I decided to try and > fix it up last night, since I had an idea that I thought would work. > > Several years ago, I read somewhere (a Microsoft Press publication of > some sort) that, even when using *synchonous* I/O, you could pass an > OVERLAPPED structure to ReadFile() and have it use the file offset in > that structure, rather than the implicit stream pointer. > > So, I modified my "hacked up test program" from the other day to try > it out; the diff looks like: > > -- >8 -- > diff --git a/test-pread.c b/test-pread.c > index 61280cb..8a2b2ff 100644 > --- a/test-pread.c > +++ b/test-pread.c > @@ -1,5 +1,6 @@ >  #include "git-compat-util.h" >  #include "thread-utils.h" > +#include > >  #define DATA_FILE "junk.data" >  #define MAX_DATA 256 * 1024 > @@ -16,6 +17,31 @@ struct thread_data { > >  static struct thread_data t[NUM_THREADS+1]; > > +ssize_t pread_(int fd, void *buf, size_t count, off_t offset) > +{ > +       OVERLAPPED o = {0}; > +       HANDLE fh = (HANDLE)_get_osfhandle(fd); > +       uint64_t off = offset; > +       DWORD bytes; > +       BOOL ret; > + > +       if (fh == INVALID_HANDLE_VALUE) { > +               errno = EBADF; > +               return -1; > +       } > + > +       o.Offset = off & 0xffffffff; > +       o.OffsetHigh = (off >> 32) & 0xffffffff; > + > +       ret = ReadFile(fh, buf, (DWORD)count, &bytes, &o); > +       if (!ret) { > +               errno = EIO; > +               return -1; > +       } > + > +       return (ssize_t)bytes; > +} > + >  int create_data_file(void) >  { >        int i, fd = open(DATA_FILE, O_CREAT | O_TRUNC | O_WRONLY, 0600); > @@ -55,7 +81,7 @@ void *pread_thread(void *data) >                ssize_t sz; >                d->n = d->n * 1103515245 + 12345; >                j = d->n % MAX_DATA; > -               sz = pread(d->fd, &rd, sizeof(int), j * sizeof(int)); > +               sz = pread_(d->fd, &rd, sizeof(int), j * sizeof(int)); >                if (sz < 0 || rd != j) >                        d->fails++; >                d->cnt++; > > -- 8< -- > > The result of running the updated program looks like: > >    ramsay $ gcc -I. -o test-pread test-pread.c >    ramsay $ ./test-pread.exe >     0: trials 524288, failed 262139 >     1: trials 500000, failed 0 >     2: trials 500000, failed 0 >     3: trials 500000, failed 0 >    ramsay $ > > So, the results are a little disappointing. :( > > Although ReadFile() is indeed using the OVERLAPPED structure to specify > the read position, it is still updating the implicit stream position. > > So, this implementation does not faithfully reproduce the full semantics > of pread(), since it updates the stream position (affecting other I/O > calls which rely on the implicit file position. eg read()). > > [Note: this was tested on Windows XP SP3. Maybe Vista/Win7/Win8 would > behave differently?] > > This implementation should be sufficient to fix the immediate problem with > the threaded index-pack, but I'm not sure it would be a good idea to > provide an *almost* compliant pread(). > > I've attached the full test program below, just for completeness. > We've been through this before: http://thread.gmane.org/CABPQNSZ6VdyoLcVUUJ4z5A2A7KGP8qBZAkzdx8zAtAs2mZN25w@mail.gmail.com My worries are the same as yours: An approach like this looks a bit too much like pread, and might make someone else assume it works the same. ```