{"thread":{"id":"30910","subject":"Attempt to fix pread() threading issue on Cygwin and MinGW","startedAt":"2012-06-26T19:00:46Z","lastAt":"2012-06-26T19:46:28Z","messageCount":2,"participants":["Ramsay Jones","Erik Faye-Lund"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"194302","messageId":"4FEA06DE.6080101@ramsay1.demon.co.uk","threadId":"30910","inReplyTo":null,"subject":"Attempt to fix pread() threading issue on Cygwin and MinGW","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-06-26T19:00:46Z","receivedAt":"2012-06-26T19:00:46Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Johannes,\n\nI had an \"fix pread() on MinGW\" item low on my todo list. As we have\nseen recently, Cygwin has the same problem. So, I decided to try and\nfix it up last night, since I had an idea that I thought would work.\n\nSeveral years ago, I read somewhere (a Microsoft Press publication of\nsome sort) that, even when using *synchonous* I/O, you could pass an\nOVERLAPPED structure to ReadFile() and have it use the file offset in\nthat structure, rather than the implicit stream pointer.\n\nSo, I modified my \"hacked up test program\" from the other day to try\nit out; the diff looks like:\n\n-- >8 --\ndiff --git a/test-pread.c b/test-pread.c\nindex 61280cb..8a2b2ff 100644\n--- a/test-pread.c\n+++ b/test-pread.c\n@@ -1,5 +1,6 @@\n #include \"git-compat-util.h\"\n #include \"thread-utils.h\"\n+#include <windows.h>\n \n #define DATA_FILE \"junk.data\"\n #define MAX_DATA 256 * 1024\n@@ -16,6 +17,31 @@ struct thread_data {\n \n static struct thread_data t[NUM_THREADS+1];\n \n+ssize_t pread_(int fd, void *buf, size_t count, off_t offset)\n+{\n+\tOVERLAPPED o = {0};\n+\tHANDLE fh = (HANDLE)_get_osfhandle(fd);\n+\tuint64_t off = offset;\n+\tDWORD bytes;\n+\tBOOL ret;\n+\n+\tif (fh == INVALID_HANDLE_VALUE) {\n+\t\terrno = EBADF;\n+\t\treturn -1;\n+\t}\n+\n+\to.Offset = off & 0xffffffff;\n+\to.OffsetHigh = (off >> 32) & 0xffffffff;\n+\n+\tret = ReadFile(fh, buf, (DWORD)count, &bytes, &o);\n+\tif (!ret) {\n+\t\terrno = EIO;\n+\t\treturn -1;\n+\t}\n+\n+\treturn (ssize_t)bytes;\n+}\n+\n int create_data_file(void)\n {\n \tint i, fd = open(DATA_FILE, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n@@ -55,7 +81,7 @@ void *pread_thread(void *data)\n \t\tssize_t sz;\n \t\td->n = d->n * 1103515245 + 12345;\n \t\tj = d->n % MAX_DATA;\n-\t\tsz = pread(d->fd, &rd, sizeof(int), j * sizeof(int));\n+\t\tsz = pread_(d->fd, &rd, sizeof(int), j * sizeof(int));\n \t\tif (sz < 0 || rd != j)\n \t\t\td->fails++;\n \t\td->cnt++;\n\n-- 8< --\n\nThe result of running the updated program looks like:\n\n    ramsay $ gcc -I. -o test-pread test-pread.c\n    ramsay $ ./test-pread.exe\n     0: trials 524288, failed 262139\n     1: trials 500000, failed 0\n     2: trials 500000, failed 0\n     3: trials 500000, failed 0\n    ramsay $\n\nSo, the results are a little disappointing. :(\n\nAlthough ReadFile() is indeed using the OVERLAPPED structure to specify\nthe read position, it is still updating the implicit stream position.\n\nSo, this implementation does not faithfully reproduce the full semantics\nof pread(), since it updates the stream position (affecting other I/O\ncalls which rely on the implicit file position. eg read()).\n\n[Note: this was tested on Windows XP SP3. Maybe Vista/Win7/Win8 would\nbehave differently?]\n\nThis implementation should be sufficient to fix the immediate problem with\nthe threaded index-pack, but I'm not sure it would be a good idea to\nprovide an *almost* compliant pread().\n\nI've attached the full test program below, just for completeness.\n\nATB,\nRamsay Jones\n\n-- >8 --\n#include \"git-compat-util.h\"\n#include \"thread-utils.h\"\n#include <windows.h>\n\n#define DATA_FILE \"junk.data\"\n#define MAX_DATA 256 * 1024\n#define NUM_THREADS 3\n#define TRIALS 500000\n\nstruct thread_data {\n\tpthread_t t;\n\tint fd;\n\tint cnt;\n\tint fails;\n\tunsigned long n;\n};\n\nstatic struct thread_data t[NUM_THREADS+1];\n\nssize_t pread_(int fd, void *buf, size_t count, off_t offset)\n{\n\tOVERLAPPED o = {0};\n\tHANDLE fh = (HANDLE)_get_osfhandle(fd);\n\tuint64_t off = offset;\n\tDWORD bytes;\n\tBOOL ret;\n\n\tif (fh == INVALID_HANDLE_VALUE) {\n\t\terrno = EBADF;\n\t\treturn -1;\n\t}\n\n\to.Offset = off & 0xffffffff;\n\to.OffsetHigh = (off >> 32) & 0xffffffff;\n\n\tret = ReadFile(fh, buf, (DWORD)count, &bytes, &o);\n\tif (!ret) {\n\t\terrno = EIO;\n\t\treturn -1;\n\t}\n\n\treturn (ssize_t)bytes;\n}\n\nint create_data_file(void)\n{\n\tint i, fd = open(DATA_FILE, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n\tif (fd < 0)\n\t\treturn -1;\n\tfor (i = 0; i < MAX_DATA; i++)\n\t\tif (write(fd, &i, sizeof(int)) < 0) {\n\t\t\tclose(fd);\n\t\t\tunlink(DATA_FILE);\n\t\t\treturn -1;\n\t\t}\n\tclose(fd);\n\treturn 0;\n}\n\nvoid *read_thread(void *data)\n{\n\tstruct thread_data *d = (struct thread_data *)data;\n\tint i, j, rd;\n\tfor (i = 0; i < TRIALS; i += MAX_DATA) {\n\t\tfor (j = 0; j < MAX_DATA; j++) {\n\t\t\tssize_t sz = read(d->fd, &rd, sizeof(int));\n\t\t\tif (sz < 0 || rd != j)\n\t\t\t\td->fails++;\n\t\t\td->cnt++;\n\t\t}\n\t\tlseek(d->fd, 0, SEEK_SET);\n\t}\n\treturn NULL;\n}\n\nvoid *pread_thread(void *data)\n{\n\tstruct thread_data *d = (struct thread_data *)data;\n\tint i, j, rd;\n\tfor (i = 0; i < TRIALS; i++) {\n\t\tssize_t sz;\n\t\td->n = d->n * 1103515245 + 12345;\n\t\tj = d->n % MAX_DATA;\n\t\tsz = pread_(d->fd, &rd, sizeof(int), j * sizeof(int));\n\t\tif (sz < 0 || rd != j)\n\t\t\td->fails++;\n\t\td->cnt++;\n\t}\n\treturn NULL;\n}\n\nint main(int argc, char *argv[])\n{\n\tint fd, i;\n\n\tif (create_data_file() < 0) {\n\t\tprintf(\"can't create data file\\n\");\n\t\treturn 1;\n\t}\n\n\tif ((fd = open(DATA_FILE, O_RDONLY)) < 0) {\n\t\tprintf(\"can't open data file\\n\");\n\t\tunlink(DATA_FILE);\n\t\treturn 1;\n\t}\n\n\tfor (i = 0; i < NUM_THREADS+1; i++) {\n\t\tint ret;\n\n\t\tt[i].fd = fd;\n\t\tt[i].cnt = 0;\n\t\tt[i].fails = 0;\n\t\tt[i].n = i * 16381;\n\t\tret = pthread_create(&t[i].t, NULL,\n\t\t\t\t(i == 0) ? read_thread : pread_thread,\n\t\t\t\t&t[i]);\n\t\tif (ret) {\n\t\t\tprintf(\"can't create thread %d (%s)\\n\", i, strerror(ret));\n\t\t\tunlink(DATA_FILE);\n\t\t\treturn 1;\n\t\t}\n\t}\n\n\tfor (i = 0; i < NUM_THREADS+1; i++)\n\t\tpthread_join(t[i].t, NULL);\n\tclose(fd);\n\n\tfor (i = 0; i < NUM_THREADS+1; i++)\n\t\tprintf(\"%2d: trials %d, failed %d\\n\", i, t[i].cnt, t[i].fails);\n\n\tunlink(DATA_FILE);\n\treturn 0;\n}\n"},{"id":"194306","messageId":"CABPQNSZWLvYGH6sFkWa588+fPJXV3c5ViKsEyMABViN74ghaLg@mail.gmail.com","threadId":"30910","inReplyTo":"4FEA06DE.6080101@ramsay1.demon.co.uk","subject":"Re: Attempt to fix pread() threading issue on Cygwin and MinGW","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-06-26T19:46:28Z","receivedAt":"2012-06-26T19:46:28Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Jun 26, 2012 at 9:00 PM, Ramsay Jones\n<ramsay@ramsay1.demon.co.uk> wrote:\n> Hi Johannes,\n>\n> I had an \"fix pread() on MinGW\" item low on my todo list. As we have\n> seen recently, Cygwin has the same problem. So, I decided to try and\n> fix it up last night, since I had an idea that I thought would work.\n>\n> Several years ago, I read somewhere (a Microsoft Press publication of\n> some sort) that, even when using *synchonous* I/O, you could pass an\n> OVERLAPPED structure to ReadFile() and have it use the file offset in\n> that structure, rather than the implicit stream pointer.\n>\n> So, I modified my \"hacked up test program\" from the other day to try\n> it out; the diff looks like:\n>\n> -- >8 --\n> diff --git a/test-pread.c b/test-pread.c\n> index 61280cb..8a2b2ff 100644\n> --- a/test-pread.c\n> +++ b/test-pread.c\n> @@ -1,5 +1,6 @@\n>  #include \"git-compat-util.h\"\n>  #include \"thread-utils.h\"\n> +#include <windows.h>\n>\n>  #define DATA_FILE \"junk.data\"\n>  #define MAX_DATA 256 * 1024\n> @@ -16,6 +17,31 @@ struct thread_data {\n>\n>  static struct thread_data t[NUM_THREADS+1];\n>\n> +ssize_t pread_(int fd, void *buf, size_t count, off_t offset)\n> +{\n> +       OVERLAPPED o = {0};\n> +       HANDLE fh = (HANDLE)_get_osfhandle(fd);\n> +       uint64_t off = offset;\n> +       DWORD bytes;\n> +       BOOL ret;\n> +\n> +       if (fh == INVALID_HANDLE_VALUE) {\n> +               errno = EBADF;\n> +               return -1;\n> +       }\n> +\n> +       o.Offset = off & 0xffffffff;\n> +       o.OffsetHigh = (off >> 32) & 0xffffffff;\n> +\n> +       ret = ReadFile(fh, buf, (DWORD)count, &bytes, &o);\n> +       if (!ret) {\n> +               errno = EIO;\n> +               return -1;\n> +       }\n> +\n> +       return (ssize_t)bytes;\n> +}\n> +\n>  int create_data_file(void)\n>  {\n>        int i, fd = open(DATA_FILE, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n> @@ -55,7 +81,7 @@ void *pread_thread(void *data)\n>                ssize_t sz;\n>                d->n = d->n * 1103515245 + 12345;\n>                j = d->n % MAX_DATA;\n> -               sz = pread(d->fd, &rd, sizeof(int), j * sizeof(int));\n> +               sz = pread_(d->fd, &rd, sizeof(int), j * sizeof(int));\n>                if (sz < 0 || rd != j)\n>                        d->fails++;\n>                d->cnt++;\n>\n> -- 8< --\n>\n> The result of running the updated program looks like:\n>\n>    ramsay $ gcc -I. -o test-pread test-pread.c\n>    ramsay $ ./test-pread.exe\n>     0: trials 524288, failed 262139\n>     1: trials 500000, failed 0\n>     2: trials 500000, failed 0\n>     3: trials 500000, failed 0\n>    ramsay $\n>\n> So, the results are a little disappointing. :(\n>\n> Although ReadFile() is indeed using the OVERLAPPED structure to specify\n> the read position, it is still updating the implicit stream position.\n>\n> So, this implementation does not faithfully reproduce the full semantics\n> of pread(), since it updates the stream position (affecting other I/O\n> calls which rely on the implicit file position. eg read()).\n>\n> [Note: this was tested on Windows XP SP3. Maybe Vista/Win7/Win8 would\n> behave differently?]\n>\n> This implementation should be sufficient to fix the immediate problem with\n> the threaded index-pack, but I'm not sure it would be a good idea to\n> provide an *almost* compliant pread().\n>\n> I've attached the full test program below, just for completeness.\n>\n\nWe've been through this before:\n\nhttp://thread.gmane.org/CABPQNSZ6VdyoLcVUUJ4z5A2A7KGP8qBZAkzdx8zAtAs2mZN25w@mail.gmail.com\n\nMy worries are the same as yours: An approach like this looks a bit\ntoo much like pread, and might make someone else assume it works the\nsame.\n"}]}