{"thread":{"id":"36214","subject":"[PATCH] Enable index-pack threading in msysgit.","startedAt":"2014-03-19T00:46:59Z","lastAt":"2014-03-26T08:35:22Z","messageCount":19,"participants":["szager@chromium.org","Duy Nguyen","Stefan Zager","Junio C Hamano","Karsten Blees","Nguyễn Thái Ngọc Duy","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"237030","messageId":"5328e903.joAd1dfenJmScBNr%szager@chromium.org","threadId":"36214","inReplyTo":null,"subject":"[PATCH] Enable index-pack threading in msysgit.","fromName":"","fromEmail":"szager@chromium.org","sentAt":"2014-03-19T00:46:59Z","receivedAt":"2014-03-19T00:46:59Z","isPatch":true,"sender":{"key":"szager@chromium.org","avatar":null},"body":"This adds a Windows implementation of pread.  Note that it is NOT\nsafe to intersperse calls to read() and pread() on a file\ndescriptor.  According to the ReadFile spec, using the 'overlapped'\nargument should not affect the implicit position pointer of the\ndescriptor.  Experiments have shown that this is, in fact, a lie.\n\nTo accomodate that fact, this change also incorporates:\n\nhttp://article.gmane.org/gmane.comp.version-control.git/196042\n\n... which gives each index-pack thread its own file descriptor.\n---\n builtin/index-pack.c | 21 ++++++++++++++++-----\n compat/mingw.c       | 31 ++++++++++++++++++++++++++++++-\n compat/mingw.h       |  3 +++\n config.mak.uname     |  1 -\n 4 files changed, 49 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 2f37a38..c02dd4c 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -51,6 +51,7 @@ struct thread_local {\n #endif\n \tstruct base_data *base_cache;\n \tsize_t base_cache_used;\n+\tint pack_fd;\n };\n \n /*\n@@ -91,7 +92,8 @@ static off_t consumed_bytes;\n static unsigned deepest_delta;\n static git_SHA_CTX input_ctx;\n static uint32_t input_crc32;\n-static int input_fd, output_fd, pack_fd;\n+static const char *curr_pack;\n+static int input_fd, output_fd;\n \n #ifndef NO_PTHREADS\n \n@@ -134,6 +136,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)\n  */\n static void init_thread(void)\n {\n+\tint i;\n \tinit_recursive_mutex(&read_mutex);\n \tpthread_mutex_init(&counter_mutex, NULL);\n \tpthread_mutex_init(&work_mutex, NULL);\n@@ -141,11 +144,17 @@ static void init_thread(void)\n \t\tpthread_mutex_init(&deepest_delta_mutex, NULL);\n \tpthread_key_create(&key, NULL);\n \tthread_data = xcalloc(nr_threads, sizeof(*thread_data));\n+\tfor (i = 0; i < nr_threads; i++) {\n+\t\tthread_data[i].pack_fd = open(curr_pack, O_RDONLY);\n+\t\tif (thread_data[i].pack_fd == -1)\n+\t\t\tdie_errno(\"unable to open %s\", curr_pack);\n+\t}\n \tthreads_active = 1;\n }\n \n static void cleanup_thread(void)\n {\n+\tint i;\n \tif (!threads_active)\n \t\treturn;\n \tthreads_active = 0;\n@@ -155,6 +164,8 @@ static void cleanup_thread(void)\n \tif (show_stat)\n \t\tpthread_mutex_destroy(&deepest_delta_mutex);\n \tpthread_key_delete(key);\n+\tfor (i = 0; i < nr_threads; i++)\n+\t\tclose(thread_data[i].pack_fd);\n \tfree(thread_data);\n }\n \n@@ -288,13 +299,13 @@ static const char *open_pack_file(const char *pack_name)\n \t\t\toutput_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);\n \t\tif (output_fd < 0)\n \t\t\tdie_errno(_(\"unable to create '%s'\"), pack_name);\n-\t\tpack_fd = output_fd;\n+\t\tnothread_data.pack_fd = output_fd;\n \t} else {\n \t\tinput_fd = open(pack_name, O_RDONLY);\n \t\tif (input_fd < 0)\n \t\t\tdie_errno(_(\"cannot open packfile '%s'\"), pack_name);\n \t\toutput_fd = -1;\n-\t\tpack_fd = input_fd;\n+\t\tnothread_data.pack_fd = input_fd;\n \t}\n \tgit_SHA1_Init(&input_ctx);\n \treturn pack_name;\n@@ -542,7 +553,7 @@ static void *unpack_data(struct object_entry *obj,\n \n \tdo {\n \t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n-\t\tn = pread(pack_fd, inbuf, n, from);\n+\t\tn = pread(get_thread_data()->pack_fd, inbuf, n, from);\n \t\tif (n < 0)\n \t\t\tdie_errno(_(\"cannot pread pack file\"));\n \t\tif (!n)\n@@ -1490,7 +1501,7 @@ static void show_pack_info(int stat_only)\n int cmd_index_pack(int argc, const char **argv, const char *prefix)\n {\n \tint i, fix_thin_pack = 0, verify = 0, stat_only = 0;\n-\tconst char *curr_pack, *curr_index;\n+\tconst char *curr_index;\n \tconst char *index_name = NULL, *pack_name = NULL;\n \tconst char *keep_name = NULL, *keep_msg = NULL;\n \tchar *index_name_buf = NULL, *keep_name_buf = NULL;\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 383cafe..6cc85d6 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -329,7 +329,36 @@ int mingw_mkdir(const char *path, int mode)\n \treturn ret;\n }\n \n-int mingw_open (const char *filename, int oflags, ...)\n+\n+ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)\n+{\n+\tHANDLE hand = (HANDLE)_get_osfhandle(fd);\n+\tif (hand == INVALID_HANDLE_VALUE) {\n+\t\terrno = EBADF;\n+\t\treturn -1;\n+\t}\n+\n+\tLARGE_INTEGER offset_value;\n+\toffset_value.QuadPart = offset;\n+\n+\tDWORD bytes_read = 0;\n+\tOVERLAPPED overlapped = {0};\n+\toverlapped.Offset = offset_value.LowPart;\n+\toverlapped.OffsetHigh = offset_value.HighPart;\n+\tBOOL result = ReadFile(hand, buf, count, &bytes_read, &overlapped);\n+\n+\tssize_t ret = bytes_read;\n+\n+\tif (!result && GetLastError() != ERROR_HANDLE_EOF)\n+\t{\n+\t\terrno = err_win_to_posix(GetLastError());\n+\t\tret = -1;\n+\t}\n+\n+\treturn ret;\n+}\n+\n+int mingw_open(const char *filename, int oflags, ...)\n {\n \tva_list args;\n \tunsigned mode;\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 08b83fe..377ba50 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -174,6 +174,9 @@ int mingw_unlink(const char *pathname);\n int mingw_rmdir(const char *path);\n #define rmdir mingw_rmdir\n \n+ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset);\n+#define pread mingw_pread\n+\n int mingw_open (const char *filename, int oflags, ...);\n #define open mingw_open\n \ndiff --git a/config.mak.uname b/config.mak.uname\nindex e8acc39..b405524 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -474,7 +474,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n endif\n ifneq (,$(findstring MINGW,$(uname_S)))\n \tpathsep = ;\n-\tNO_PREAD = YesPlease\n \tNEEDS_CRYPTO_WITH_SSL = YesPlease\n \tNO_LIBGEN_H = YesPlease\n \tNO_POLL = YesPlease\n-- \n1.9.0.279.gdc9e3eb\n"},{"id":"237042","messageId":"CACsJy8BOZa6vJU_s9sxYrtSdpL-4PDTpbo6r6TC8z2LD1GtkMQ@mail.gmail.com","threadId":"36214","inReplyTo":"5328e903.joAd1dfenJmScBNr%szager@chromium.org","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-19T07:30:51Z","receivedAt":"2014-03-19T07:30:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 19, 2014 at 7:46 AM,  <szager@chromium.org> wrote:\n> This adds a Windows implementation of pread.  Note that it is NOT\n> safe to intersperse calls to read() and pread() on a file\n> descriptor.  According to the ReadFile spec, using the 'overlapped'\n> argument should not affect the implicit position pointer of the\n> descriptor.  Experiments have shown that this is, in fact, a lie.\n\nIf I understand it correctly, new pread() is added because\ncompat/pread.c does not work because of some other read() in between?\nWhere are those read() (I can only see one in index-pack.c, but there\ncould be some hidden read()..)\n\n>\n> To accomodate that fact, this change also incorporates:\n>\n> http://article.gmane.org/gmane.comp.version-control.git/196042\n>\n> ... which gives each index-pack thread its own file descriptor.\n> ---\n>  builtin/index-pack.c | 21 ++++++++++++++++-----\n>  compat/mingw.c       | 31 ++++++++++++++++++++++++++++++-\n>  compat/mingw.h       |  3 +++\n>  config.mak.uname     |  1 -\n>  4 files changed, 49 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 2f37a38..c02dd4c 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -51,6 +51,7 @@ struct thread_local {\n>  #endif\n>         struct base_data *base_cache;\n>         size_t base_cache_used;\n> +       int pack_fd;\n>  };\n>\n>  /*\n> @@ -91,7 +92,8 @@ static off_t consumed_bytes;\n>  static unsigned deepest_delta;\n>  static git_SHA_CTX input_ctx;\n>  static uint32_t input_crc32;\n> -static int input_fd, output_fd, pack_fd;\n> +static const char *curr_pack;\n> +static int input_fd, output_fd;\n>\n>  #ifndef NO_PTHREADS\n>\n> @@ -134,6 +136,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)\n>   */\n>  static void init_thread(void)\n>  {\n> +       int i;\n>         init_recursive_mutex(&read_mutex);\n>         pthread_mutex_init(&counter_mutex, NULL);\n>         pthread_mutex_init(&work_mutex, NULL);\n> @@ -141,11 +144,17 @@ static void init_thread(void)\n>                 pthread_mutex_init(&deepest_delta_mutex, NULL);\n>         pthread_key_create(&key, NULL);\n>         thread_data = xcalloc(nr_threads, sizeof(*thread_data));\n> +       for (i = 0; i < nr_threads; i++) {\n> +               thread_data[i].pack_fd = open(curr_pack, O_RDONLY);\n> +               if (thread_data[i].pack_fd == -1)\n> +                       die_errno(\"unable to open %s\", curr_pack);\n> +       }\n>         threads_active = 1;\n>  }\n>\n>  static void cleanup_thread(void)\n>  {\n> +       int i;\n>         if (!threads_active)\n>                 return;\n>         threads_active = 0;\n> @@ -155,6 +164,8 @@ static void cleanup_thread(void)\n>         if (show_stat)\n>                 pthread_mutex_destroy(&deepest_delta_mutex);\n>         pthread_key_delete(key);\n> +       for (i = 0; i < nr_threads; i++)\n> +               close(thread_data[i].pack_fd);\n>         free(thread_data);\n>  }\n>\n> @@ -288,13 +299,13 @@ static const char *open_pack_file(const char *pack_name)\n>                         output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);\n>                 if (output_fd < 0)\n>                         die_errno(_(\"unable to create '%s'\"), pack_name);\n> -               pack_fd = output_fd;\n> +               nothread_data.pack_fd = output_fd;\n>         } else {\n>                 input_fd = open(pack_name, O_RDONLY);\n>                 if (input_fd < 0)\n>                         die_errno(_(\"cannot open packfile '%s'\"), pack_name);\n>                 output_fd = -1;\n> -               pack_fd = input_fd;\n> +               nothread_data.pack_fd = input_fd;\n>         }\n>         git_SHA1_Init(&input_ctx);\n>         return pack_name;\n> @@ -542,7 +553,7 @@ static void *unpack_data(struct object_entry *obj,\n>\n>         do {\n>                 ssize_t n = (len < 64*1024) ? len : 64*1024;\n> -               n = pread(pack_fd, inbuf, n, from);\n> +               n = pread(get_thread_data()->pack_fd, inbuf, n, from);\n>                 if (n < 0)\n>                         die_errno(_(\"cannot pread pack file\"));\n>                 if (!n)\n> @@ -1490,7 +1501,7 @@ static void show_pack_info(int stat_only)\n>  int cmd_index_pack(int argc, const char **argv, const char *prefix)\n>  {\n>         int i, fix_thin_pack = 0, verify = 0, stat_only = 0;\n> -       const char *curr_pack, *curr_index;\n> +       const char *curr_index;\n>         const char *index_name = NULL, *pack_name = NULL;\n>         const char *keep_name = NULL, *keep_msg = NULL;\n>         char *index_name_buf = NULL, *keep_name_buf = NULL;\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 383cafe..6cc85d6 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -329,7 +329,36 @@ int mingw_mkdir(const char *path, int mode)\n>         return ret;\n>  }\n>\n> -int mingw_open (const char *filename, int oflags, ...)\n> +\n> +ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)\n> +{\n> +       HANDLE hand = (HANDLE)_get_osfhandle(fd);\n> +       if (hand == INVALID_HANDLE_VALUE) {\n> +               errno = EBADF;\n> +               return -1;\n> +       }\n> +\n> +       LARGE_INTEGER offset_value;\n> +       offset_value.QuadPart = offset;\n> +\n> +       DWORD bytes_read = 0;\n> +       OVERLAPPED overlapped = {0};\n> +       overlapped.Offset = offset_value.LowPart;\n> +       overlapped.OffsetHigh = offset_value.HighPart;\n> +       BOOL result = ReadFile(hand, buf, count, &bytes_read, &overlapped);\n> +\n> +       ssize_t ret = bytes_read;\n> +\n> +       if (!result && GetLastError() != ERROR_HANDLE_EOF)\n> +       {\n> +               errno = err_win_to_posix(GetLastError());\n> +               ret = -1;\n> +       }\n> +\n> +       return ret;\n> +}\n> +\n> +int mingw_open(const char *filename, int oflags, ...)\n>  {\n>         va_list args;\n>         unsigned mode;\n> diff --git a/compat/mingw.h b/compat/mingw.h\n> index 08b83fe..377ba50 100644\n> --- a/compat/mingw.h\n> +++ b/compat/mingw.h\n> @@ -174,6 +174,9 @@ int mingw_unlink(const char *pathname);\n>  int mingw_rmdir(const char *path);\n>  #define rmdir mingw_rmdir\n>\n> +ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset);\n> +#define pread mingw_pread\n> +\n>  int mingw_open (const char *filename, int oflags, ...);\n>  #define open mingw_open\n>\n> diff --git a/config.mak.uname b/config.mak.uname\n> index e8acc39..b405524 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -474,7 +474,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n>  endif\n>  ifneq (,$(findstring MINGW,$(uname_S)))\n>         pathsep = ;\n> -       NO_PREAD = YesPlease\n>         NEEDS_CRYPTO_WITH_SSL = YesPlease\n>         NO_LIBGEN_H = YesPlease\n>         NO_POLL = YesPlease\n> --\n> 1.9.0.279.gdc9e3eb\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\n\n\n-- \nDuy\n"},{"id":"237044","messageId":"CAHOQ7J_Wmjo6AJRQra2UDWX3WRboD+-4SaGCHYOUgRR+NyUX4A@mail.gmail.com","threadId":"36214","inReplyTo":"CACsJy8BOZa6vJU_s9sxYrtSdpL-4PDTpbo6r6TC8z2LD1GtkMQ@mail.gmail.com","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Stefan Zager","fromEmail":"szager@chromium.org","sentAt":"2014-03-19T07:50:44Z","receivedAt":"2014-03-19T07:50:44Z","isPatch":true,"sender":{"key":"szager@chromium.org","avatar":null},"body":"On Wed, Mar 19, 2014 at 12:30 AM, Duy Nguyen <pclouds@gmail.com> wrote:\n> On Wed, Mar 19, 2014 at 7:46 AM,  <szager@chromium.org> wrote:\n>> This adds a Windows implementation of pread.  Note that it is NOT\n>> safe to intersperse calls to read() and pread() on a file\n>> descriptor.  According to the ReadFile spec, using the 'overlapped'\n>> argument should not affect the implicit position pointer of the\n>> descriptor.  Experiments have shown that this is, in fact, a lie.\n>\n> If I understand it correctly, new pread() is added because\n> compat/pread.c does not work because of some other read() in between?\n> Where are those read() (I can only see one in index-pack.c, but there\n> could be some hidden read()..)\n\nI *think* it's the call to fixup_pack_header_footer(), but I'm not 100% sure.\n\nI suppose it would be possible to fix the immediate problem just by\nusing one fd per thread, without a new pread implementation.  But it\nseems better overall to have a pread() implementation that is\nthread-safe as long as read() and pread() aren't interspersed; and\nthen convert all existing read() calls to pread().  That would be a\ngood follow-up patch...\n\n\n>\n>>\n>> To accomodate that fact, this change also incorporates:\n>>\n>> http://article.gmane.org/gmane.comp.version-control.git/196042\n>>\n>> ... which gives each index-pack thread its own file descriptor.\n>> ---\n>>  builtin/index-pack.c | 21 ++++++++++++++++-----\n>>  compat/mingw.c       | 31 ++++++++++++++++++++++++++++++-\n>>  compat/mingw.h       |  3 +++\n>>  config.mak.uname     |  1 -\n>>  4 files changed, 49 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n>> index 2f37a38..c02dd4c 100644\n>> --- a/builtin/index-pack.c\n>> +++ b/builtin/index-pack.c\n>> @@ -51,6 +51,7 @@ struct thread_local {\n>>  #endif\n>>         struct base_data *base_cache;\n>>         size_t base_cache_used;\n>> +       int pack_fd;\n>>  };\n>>\n>>  /*\n>> @@ -91,7 +92,8 @@ static off_t consumed_bytes;\n>>  static unsigned deepest_delta;\n>>  static git_SHA_CTX input_ctx;\n>>  static uint32_t input_crc32;\n>> -static int input_fd, output_fd, pack_fd;\n>> +static const char *curr_pack;\n>> +static int input_fd, output_fd;\n>>\n>>  #ifndef NO_PTHREADS\n>>\n>> @@ -134,6 +136,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)\n>>   */\n>>  static void init_thread(void)\n>>  {\n>> +       int i;\n>>         init_recursive_mutex(&read_mutex);\n>>         pthread_mutex_init(&counter_mutex, NULL);\n>>         pthread_mutex_init(&work_mutex, NULL);\n>> @@ -141,11 +144,17 @@ static void init_thread(void)\n>>                 pthread_mutex_init(&deepest_delta_mutex, NULL);\n>>         pthread_key_create(&key, NULL);\n>>         thread_data = xcalloc(nr_threads, sizeof(*thread_data));\n>> +       for (i = 0; i < nr_threads; i++) {\n>> +               thread_data[i].pack_fd = open(curr_pack, O_RDONLY);\n>> +               if (thread_data[i].pack_fd == -1)\n>> +                       die_errno(\"unable to open %s\", curr_pack);\n>> +       }\n>>         threads_active = 1;\n>>  }\n>>\n>>  static void cleanup_thread(void)\n>>  {\n>> +       int i;\n>>         if (!threads_active)\n>>                 return;\n>>         threads_active = 0;\n>> @@ -155,6 +164,8 @@ static void cleanup_thread(void)\n>>         if (show_stat)\n>>                 pthread_mutex_destroy(&deepest_delta_mutex);\n>>         pthread_key_delete(key);\n>> +       for (i = 0; i < nr_threads; i++)\n>> +               close(thread_data[i].pack_fd);\n>>         free(thread_data);\n>>  }\n>>\n>> @@ -288,13 +299,13 @@ static const char *open_pack_file(const char *pack_name)\n>>                         output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);\n>>                 if (output_fd < 0)\n>>                         die_errno(_(\"unable to create '%s'\"), pack_name);\n>> -               pack_fd = output_fd;\n>> +               nothread_data.pack_fd = output_fd;\n>>         } else {\n>>                 input_fd = open(pack_name, O_RDONLY);\n>>                 if (input_fd < 0)\n>>                         die_errno(_(\"cannot open packfile '%s'\"), pack_name);\n>>                 output_fd = -1;\n>> -               pack_fd = input_fd;\n>> +               nothread_data.pack_fd = input_fd;\n>>         }\n>>         git_SHA1_Init(&input_ctx);\n>>         return pack_name;\n>> @@ -542,7 +553,7 @@ static void *unpack_data(struct object_entry *obj,\n>>\n>>         do {\n>>                 ssize_t n = (len < 64*1024) ? len : 64*1024;\n>> -               n = pread(pack_fd, inbuf, n, from);\n>> +               n = pread(get_thread_data()->pack_fd, inbuf, n, from);\n>>                 if (n < 0)\n>>                         die_errno(_(\"cannot pread pack file\"));\n>>                 if (!n)\n>> @@ -1490,7 +1501,7 @@ static void show_pack_info(int stat_only)\n>>  int cmd_index_pack(int argc, const char **argv, const char *prefix)\n>>  {\n>>         int i, fix_thin_pack = 0, verify = 0, stat_only = 0;\n>> -       const char *curr_pack, *curr_index;\n>> +       const char *curr_index;\n>>         const char *index_name = NULL, *pack_name = NULL;\n>>         const char *keep_name = NULL, *keep_msg = NULL;\n>>         char *index_name_buf = NULL, *keep_name_buf = NULL;\n>> diff --git a/compat/mingw.c b/compat/mingw.c\n>> index 383cafe..6cc85d6 100644\n>> --- a/compat/mingw.c\n>> +++ b/compat/mingw.c\n>> @@ -329,7 +329,36 @@ int mingw_mkdir(const char *path, int mode)\n>>         return ret;\n>>  }\n>>\n>> -int mingw_open (const char *filename, int oflags, ...)\n>> +\n>> +ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)\n>> +{\n>> +       HANDLE hand = (HANDLE)_get_osfhandle(fd);\n>> +       if (hand == INVALID_HANDLE_VALUE) {\n>> +               errno = EBADF;\n>> +               return -1;\n>> +       }\n>> +\n>> +       LARGE_INTEGER offset_value;\n>> +       offset_value.QuadPart = offset;\n>> +\n>> +       DWORD bytes_read = 0;\n>> +       OVERLAPPED overlapped = {0};\n>> +       overlapped.Offset = offset_value.LowPart;\n>> +       overlapped.OffsetHigh = offset_value.HighPart;\n>> +       BOOL result = ReadFile(hand, buf, count, &bytes_read, &overlapped);\n>> +\n>> +       ssize_t ret = bytes_read;\n>> +\n>> +       if (!result && GetLastError() != ERROR_HANDLE_EOF)\n>> +       {\n>> +               errno = err_win_to_posix(GetLastError());\n>> +               ret = -1;\n>> +       }\n>> +\n>> +       return ret;\n>> +}\n>> +\n>> +int mingw_open(const char *filename, int oflags, ...)\n>>  {\n>>         va_list args;\n>>         unsigned mode;\n>> diff --git a/compat/mingw.h b/compat/mingw.h\n>> index 08b83fe..377ba50 100644\n>> --- a/compat/mingw.h\n>> +++ b/compat/mingw.h\n>> @@ -174,6 +174,9 @@ int mingw_unlink(const char *pathname);\n>>  int mingw_rmdir(const char *path);\n>>  #define rmdir mingw_rmdir\n>>\n>> +ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset);\n>> +#define pread mingw_pread\n>> +\n>>  int mingw_open (const char *filename, int oflags, ...);\n>>  #define open mingw_open\n>>\n>> diff --git a/config.mak.uname b/config.mak.uname\n>> index e8acc39..b405524 100644\n>> --- a/config.mak.uname\n>> +++ b/config.mak.uname\n>> @@ -474,7 +474,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n>>  endif\n>>  ifneq (,$(findstring MINGW,$(uname_S)))\n>>         pathsep = ;\n>> -       NO_PREAD = YesPlease\n>>         NEEDS_CRYPTO_WITH_SSL = YesPlease\n>>         NO_LIBGEN_H = YesPlease\n>>         NO_POLL = YesPlease\n>> --\n>> 1.9.0.279.gdc9e3eb\n>>\n>> --\n>> To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> the body of a message to majordomo@vger.kernel.org\n>> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n>\n>\n>\n> --\n> Duy\n"},{"id":"237055","messageId":"CACsJy8A7ESSjfHqr96_yYjNsE-A1Sf=8+rmRfGrjML0+fCWTTg@mail.gmail.com","threadId":"36214","inReplyTo":"CAHOQ7J_Wmjo6AJRQra2UDWX3WRboD+-4SaGCHYOUgRR+NyUX4A@mail.gmail.com","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-19T10:28:27Z","receivedAt":"2014-03-19T10:28:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 19, 2014 at 2:50 PM, Stefan Zager <szager@chromium.org> wrote:\n> On Wed, Mar 19, 2014 at 12:30 AM, Duy Nguyen <pclouds@gmail.com> wrote:\n>> On Wed, Mar 19, 2014 at 7:46 AM,  <szager@chromium.org> wrote:\n>>> This adds a Windows implementation of pread.  Note that it is NOT\n>>> safe to intersperse calls to read() and pread() on a file\n>>> descriptor.  According to the ReadFile spec, using the 'overlapped'\n>>> argument should not affect the implicit position pointer of the\n>>> descriptor.  Experiments have shown that this is, in fact, a lie.\n>>\n>> If I understand it correctly, new pread() is added because\n>> compat/pread.c does not work because of some other read() in between?\n>> Where are those read() (I can only see one in index-pack.c, but there\n>> could be some hidden read()..)\n>\n> I *think* it's the call to fixup_pack_header_footer(), but I'm not 100% sure.\n>\n> I suppose it would be possible to fix the immediate problem just by\n> using one fd per thread, without a new pread implementation.  But it\n> seems better overall to have a pread() implementation that is\n> thread-safe as long as read() and pread() aren't interspersed; and\n> then convert all existing read() calls to pread().  That would be a\n> good follow-up patch...\n\nI still don't understand how compat/pread.c does not work with pack_fd\nper thread. I don't have Windows to test, but I forced compat/pread.c\non on Linux with similar pack_fd changes and it worked fine, helgrind\nonly complained about progress.c.\n\nA pread() implementation that is thread-safe with condition sounds\nlike an invite for trouble later. And I don't think converting read()\nto pread() is a good idea. Platforms that rely on pread() will hit\nfirst because of more use of compat/pread.c. read() seeks while\npread() does not, so we have to audit more code..\n-- \nDuy\n"},{"id":"237083","messageId":"CAHOQ7J9c_ZfzYEmO861Oa64YZeArQQBMnah1yWAkChME7dA+TA@mail.gmail.com","threadId":"36214","inReplyTo":"CACsJy8A7ESSjfHqr96_yYjNsE-A1Sf=8+rmRfGrjML0+fCWTTg@mail.gmail.com","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Stefan Zager","fromEmail":"szager@chromium.org","sentAt":"2014-03-19T16:57:02Z","receivedAt":"2014-03-19T16:57:02Z","isPatch":true,"sender":{"key":"szager@chromium.org","avatar":null},"body":"On Wed, Mar 19, 2014 at 3:28 AM, Duy Nguyen <pclouds@gmail.com> wrote:\n> On Wed, Mar 19, 2014 at 2:50 PM, Stefan Zager <szager@chromium.org> wrote:\n>>\n>> I suppose it would be possible to fix the immediate problem just by\n>> using one fd per thread, without a new pread implementation.  But it\n>> seems better overall to have a pread() implementation that is\n>> thread-safe as long as read() and pread() aren't interspersed; and\n>> then convert all existing read() calls to pread().  That would be a\n>> good follow-up patch...\n>\n> I still don't understand how compat/pread.c does not work with pack_fd\n> per thread. I don't have Windows to test, but I forced compat/pread.c\n> on on Linux with similar pack_fd changes and it worked fine, helgrind\n> only complained about progress.c.\n>\n> A pread() implementation that is thread-safe with condition sounds\n> like an invite for trouble later. And I don't think converting read()\n> to pread() is a good idea. Platforms that rely on pread() will hit\n> first because of more use of compat/pread.c. read() seeks while\n> pread() does not, so we have to audit more code..\n\nUsing one fd per thread is all well and good for something like\nindex-pack, which only accesses a single pack file.  But using that\nheuristic to add threading elsewhere is probably not going to work.\nFor example, I have a patch in progress to add threading to checkout,\nand another one planned to add threading to status.  In both cases, we\nwould need one fd per thread per pack file, which is pretty\nridiculous.\n\nThere really aren't very many calls to read() in the code.  I don't\nthink it would be very difficult to eliminate the remaining ones.  The\nmore interesting question, I think is: what platforms still don't have\na thread-safe pread implementation?\n"},{"id":"237104","messageId":"CAHOQ7J9sJv-L2xeKOiq1YvG5HhUP_XWCEdrtJogfZW6-NaDmWA@mail.gmail.com","threadId":"36214","inReplyTo":"CAHOQ7J9c_ZfzYEmO861Oa64YZeArQQBMnah1yWAkChME7dA+TA@mail.gmail.com","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Stefan Zager","fromEmail":"szager@chromium.org","sentAt":"2014-03-19T19:15:41Z","receivedAt":"2014-03-19T19:15:41Z","isPatch":true,"sender":{"key":"szager@chromium.org","avatar":null},"body":"On Wed, Mar 19, 2014 at 9:57 AM, Stefan Zager <szager@chromium.org> wrote:\n>>\n>> I still don't understand how compat/pread.c does not work with pack_fd\n>> per thread. I don't have Windows to test, but I forced compat/pread.c\n>> on on Linux with similar pack_fd changes and it worked fine, helgrind\n>> only complained about progress.c.\n>>\n>> A pread() implementation that is thread-safe with condition sounds\n>> like an invite for trouble later. And I don't think converting read()\n>> to pread() is a good idea. Platforms that rely on pread() will hit\n>> first because of more use of compat/pread.c. read() seeks while\n>> pread() does not, so we have to audit more code..\n>\n> Using one fd per thread is all well and good for something like\n> index-pack, which only accesses a single pack file.  But using that\n> heuristic to add threading elsewhere is probably not going to work.\n> For example, I have a patch in progress to add threading to checkout,\n> and another one planned to add threading to status.  In both cases, we\n> would need one fd per thread per pack file, which is pretty\n> ridiculous.\n>\n> There really aren't very many calls to read() in the code.  I don't\n> think it would be very difficult to eliminate the remaining ones.  The\n> more interesting question, I think is: what platforms still don't have\n> a thread-safe pread implementation?\n\nI don't want to go too deep down the rabbit hole here.  We don't have\nto solve the read() vs. pread() issue once for all right now; that can\nwait for another day.  The pread() implementation in this patch is\ncertainly no worse than the one in compat/pread.c.\n\nStefan\n"},{"id":"237113","messageId":"xmqqbnx13nzq.fsf@gitster.dls.corp.google.com","threadId":"36214","inReplyTo":"5328e903.joAd1dfenJmScBNr%szager@chromium.org","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-19T20:57:45Z","receivedAt":"2014-03-19T20:57:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"szager@chromium.org writes:\n\n> This adds a Windows implementation of pread.  Note that it is NOT\n> safe to intersperse calls to read() and pread() on a file\n> descriptor.  According to the ReadFile spec, using the 'overlapped'\n> argument should not affect the implicit position pointer of the\n> descriptor.  Experiments have shown that this is, in fact, a lie.\n>\n> To accomodate that fact, this change also incorporates:\n>\n> http://article.gmane.org/gmane.comp.version-control.git/196042\n>\n> ... which gives each index-pack thread its own file descriptor.\n> ---\n\nSign-off?\n\nThe new \"per-thread file descriptors to the same thing\" in a generic\ncodepath is a bit of eyesore.  For index-pack, keeping as many file\ndescritors open to the current pack as the worker threads are should\nnot be too bad, but could we have some comment next to the field\ndefinition please (e.g. \"Windows emulation of pread() needs separate\nfd per thread; see $URL for details\" or something)?\n\n>  builtin/index-pack.c | 21 ++++++++++++++++-----\n>  compat/mingw.c       | 31 ++++++++++++++++++++++++++++++-\n>  compat/mingw.h       |  3 +++\n>  config.mak.uname     |  1 -\n>  4 files changed, 49 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 2f37a38..c02dd4c 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -51,6 +51,7 @@ struct thread_local {\n>  #endif\n>  \tstruct base_data *base_cache;\n>  \tsize_t base_cache_used;\n> +\tint pack_fd;\n>  };\n>  \n>  /*\n> @@ -91,7 +92,8 @@ static off_t consumed_bytes;\n>  static unsigned deepest_delta;\n>  static git_SHA_CTX input_ctx;\n>  static uint32_t input_crc32;\n> -static int input_fd, output_fd, pack_fd;\n> +static const char *curr_pack;\n> +static int input_fd, output_fd;\n>  \n>  #ifndef NO_PTHREADS\n>  \n> @@ -134,6 +136,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)\n>   */\n>  static void init_thread(void)\n>  {\n> +\tint i;\n>  \tinit_recursive_mutex(&read_mutex);\n>  \tpthread_mutex_init(&counter_mutex, NULL);\n>  \tpthread_mutex_init(&work_mutex, NULL);\n> @@ -141,11 +144,17 @@ static void init_thread(void)\n>  \t\tpthread_mutex_init(&deepest_delta_mutex, NULL);\n>  \tpthread_key_create(&key, NULL);\n>  \tthread_data = xcalloc(nr_threads, sizeof(*thread_data));\n> +\tfor (i = 0; i < nr_threads; i++) {\n> +\t\tthread_data[i].pack_fd = open(curr_pack, O_RDONLY);\n> +\t\tif (thread_data[i].pack_fd == -1)\n> +\t\t\tdie_errno(\"unable to open %s\", curr_pack);\n> +\t}\n>  \tthreads_active = 1;\n>  }\n>  \n>  static void cleanup_thread(void)\n>  {\n> +\tint i;\n>  \tif (!threads_active)\n>  \t\treturn;\n>  \tthreads_active = 0;\n> @@ -155,6 +164,8 @@ static void cleanup_thread(void)\n>  \tif (show_stat)\n>  \t\tpthread_mutex_destroy(&deepest_delta_mutex);\n>  \tpthread_key_delete(key);\n> +\tfor (i = 0; i < nr_threads; i++)\n> +\t\tclose(thread_data[i].pack_fd);\n>  \tfree(thread_data);\n>  }\n>  \n> @@ -288,13 +299,13 @@ static const char *open_pack_file(const char *pack_name)\n>  \t\t\toutput_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);\n>  \t\tif (output_fd < 0)\n>  \t\t\tdie_errno(_(\"unable to create '%s'\"), pack_name);\n> -\t\tpack_fd = output_fd;\n> +\t\tnothread_data.pack_fd = output_fd;\n>  \t} else {\n>  \t\tinput_fd = open(pack_name, O_RDONLY);\n>  \t\tif (input_fd < 0)\n>  \t\t\tdie_errno(_(\"cannot open packfile '%s'\"), pack_name);\n>  \t\toutput_fd = -1;\n> -\t\tpack_fd = input_fd;\n> +\t\tnothread_data.pack_fd = input_fd;\n>  \t}\n>  \tgit_SHA1_Init(&input_ctx);\n>  \treturn pack_name;\n> @@ -542,7 +553,7 @@ static void *unpack_data(struct object_entry *obj,\n>  \n>  \tdo {\n>  \t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n> -\t\tn = pread(pack_fd, inbuf, n, from);\n> +\t\tn = pread(get_thread_data()->pack_fd, inbuf, n, from);\n>  \t\tif (n < 0)\n>  \t\t\tdie_errno(_(\"cannot pread pack file\"));\n>  \t\tif (!n)\n> @@ -1490,7 +1501,7 @@ static void show_pack_info(int stat_only)\n>  int cmd_index_pack(int argc, const char **argv, const char *prefix)\n>  {\n>  \tint i, fix_thin_pack = 0, verify = 0, stat_only = 0;\n> -\tconst char *curr_pack, *curr_index;\n> +\tconst char *curr_index;\n>  \tconst char *index_name = NULL, *pack_name = NULL;\n>  \tconst char *keep_name = NULL, *keep_msg = NULL;\n>  \tchar *index_name_buf = NULL, *keep_name_buf = NULL;\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 383cafe..6cc85d6 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -329,7 +329,36 @@ int mingw_mkdir(const char *path, int mode)\n>  \treturn ret;\n>  }\n>  \n> -int mingw_open (const char *filename, int oflags, ...)\n> +\n> +ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)\n> +{\n> +\tHANDLE hand = (HANDLE)_get_osfhandle(fd);\n> +\tif (hand == INVALID_HANDLE_VALUE) {\n> +\t\terrno = EBADF;\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tLARGE_INTEGER offset_value;\n> +\toffset_value.QuadPart = offset;\n> +\n> +\tDWORD bytes_read = 0;\n> +\tOVERLAPPED overlapped = {0};\n> +\toverlapped.Offset = offset_value.LowPart;\n> +\toverlapped.OffsetHigh = offset_value.HighPart;\n> +\tBOOL result = ReadFile(hand, buf, count, &bytes_read, &overlapped);\n> +\n> +\tssize_t ret = bytes_read;\n> +\n> +\tif (!result && GetLastError() != ERROR_HANDLE_EOF)\n> +\t{\n> +\t\terrno = err_win_to_posix(GetLastError());\n> +\t\tret = -1;\n> +\t}\n> +\n> +\treturn ret;\n> +}\n> +\n> +int mingw_open(const char *filename, int oflags, ...)\n>  {\n>  \tva_list args;\n>  \tunsigned mode;\n> diff --git a/compat/mingw.h b/compat/mingw.h\n> index 08b83fe..377ba50 100644\n> --- a/compat/mingw.h\n> +++ b/compat/mingw.h\n> @@ -174,6 +174,9 @@ int mingw_unlink(const char *pathname);\n>  int mingw_rmdir(const char *path);\n>  #define rmdir mingw_rmdir\n>  \n> +ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset);\n> +#define pread mingw_pread\n> +\n>  int mingw_open (const char *filename, int oflags, ...);\n>  #define open mingw_open\n>  \n> diff --git a/config.mak.uname b/config.mak.uname\n> index e8acc39..b405524 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -474,7 +474,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n>  endif\n>  ifneq (,$(findstring MINGW,$(uname_S)))\n>  \tpathsep = ;\n> -\tNO_PREAD = YesPlease\n>  \tNEEDS_CRYPTO_WITH_SSL = YesPlease\n>  \tNO_LIBGEN_H = YesPlease\n>  \tNO_POLL = YesPlease\n"},{"id":"237174","messageId":"532AF304.7040301@gmail.com","threadId":"36214","inReplyTo":"5328e903.joAd1dfenJmScBNr%szager@chromium.org","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-03-20T13:54:12Z","receivedAt":"2014-03-20T13:54:12Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 19.03.2014 01:46, schrieb szager@chromium.org:\n> This adds a Windows implementation of pread.  Note that it is NOT\n> safe to intersperse calls to read() and pread() on a file\n> descriptor.\n\nThis is a bad idea. You're basically fixing the multi-threaded issue twice, while at the same time breaking single-threaded read/pread interop on the mingw and msvc platform. Users of pread already have to take care that its not thread-safe on some platforms, now you're adding another breakage that has to be considered in future development.\n\nThe mingw_pread implementation in [1] is both thread-safe and allows mixing read/pread in single-threaded scenarios, why not use this instead?\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/242120\n\n> \n> http://article.gmane.org/gmane.comp.version-control.git/196042\n> \n\nDuy's patch alone enables multi-threaded index-pack on all platforms (including cygwin), so IMO this should be a separate patch.\n\n> +\tif (hand == INVALID_HANDLE_VALUE) {\n> +\t\terrno = EBADF;\n> +\t\treturn -1;\n> +\t}\n\nThis check is redundant, ReadFile already ckecks for invalid handles and err_win_to_posix converts to EBADF.\n\n> +\n> +\tLARGE_INTEGER offset_value;\n> +\toffset_value.QuadPart = offset;\n> +\n> +\tDWORD bytes_read = 0;\n> +\tOVERLAPPED overlapped = {0};\n> +\toverlapped.Offset = offset_value.LowPart;\n> +\toverlapped.OffsetHigh = offset_value.HighPart;\n> +\tBOOL result = ReadFile(hand, buf, count, &bytes_read, &overlapped);\n> +\n> +\tssize_t ret = bytes_read;\n> +\n> +\tif (!result && GetLastError() != ERROR_HANDLE_EOF)\n\nAccording to MSDN docs, ReadFile never fails with ERROR_HANDLE_EOF, or is this another case where the documentation is wrong?\n\n\"When a synchronous read operation reaches the end of a file, ReadFile returns TRUE and sets *lpNumberOfBytesRead to zero.\"\n\nKarsten\n"},{"id":"237176","messageId":"CAHOQ7J9drXwcTt4b0Tcyw97KTGcifwsO5rtFNQYf7CVr3WD7zQ@mail.gmail.com","threadId":"36214","inReplyTo":"532AF304.7040301@gmail.com","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Stefan Zager","fromEmail":"szager@chromium.org","sentAt":"2014-03-20T16:08:04Z","receivedAt":"2014-03-20T16:08:04Z","isPatch":true,"sender":{"key":"szager@chromium.org","avatar":null},"body":"On Thu, Mar 20, 2014 at 6:54 AM, Karsten Blees <karsten.blees@gmail.com> wrote:\n> Am 19.03.2014 01:46, schrieb szager@chromium.org:\n>> This adds a Windows implementation of pread.  Note that it is NOT\n>> safe to intersperse calls to read() and pread() on a file\n>> descriptor.\n>\n> This is a bad idea. You're basically fixing the multi-threaded issue twice, while at the same time breaking single-threaded read/pread interop on the mingw and msvc platform. Users of pread already have to take care that its not thread-safe on some platforms, now you're adding another breakage that has to be considered in future development.\n>\n> The mingw_pread implementation in [1] is both thread-safe and allows mixing read/pread in single-threaded scenarios, why not use this instead?\n>\n> [1] http://article.gmane.org/gmane.comp.version-control.git/242120\n\n\nThat's not thread-safe.  There is, presumably, a point between the\nfirst and second calls to lseek64 when the implicit position pointer\nis incorrect.  If another thread executes its first call to lseek64 at\nthat time, then the file descriptor may end up with an incorrect\nposition pointer after all threads have finished.\n\n> Duy's patch alone enables multi-threaded index-pack on all platforms (including cygwin), so IMO this should be a separate patch.\n\nFair enough, and it's a good first step.  I would love to see it\nlanded soon.  On the Chrome project, we're currently distributing a\npatched version of msysgit; I would very much like for us to stop\ndoing that, but the performance penalty is too significant right now.\n\nDuy, would you like to re-post your patch without the new pread implementation?\n\nGoing forward, there is still a lot of performance that gets left on\nthe table when you rule out threaded file access.  There are not so\nmany calls to read, mmap, and pread in the code; it should be possible\nto rationalize them and make them thread-safe -- at least, thread-safe\nfor posix-compliant systems and msysgit, which covers the great\nmajority of git users, I would hope.\n"},{"id":"237211","messageId":"532B5F0D.2070300@gmail.com","threadId":"36214","inReplyTo":"CAHOQ7J9drXwcTt4b0Tcyw97KTGcifwsO5rtFNQYf7CVr3WD7zQ@mail.gmail.com","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-03-20T21:35:09Z","receivedAt":"2014-03-20T21:35:09Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 20.03.2014 17:08, schrieb Stefan Zager:\n> On Thu, Mar 20, 2014 at 6:54 AM, Karsten Blees <karsten.blees@gmail.com> wrote:\n>> Am 19.03.2014 01:46, schrieb szager@chromium.org:\n>>> This adds a Windows implementation of pread.  Note that it is NOT\n>>> safe to intersperse calls to read() and pread() on a file\n>>> descriptor.\n>>\n>> This is a bad idea. You're basically fixing the multi-threaded issue twice, while at the same time breaking single-threaded read/pread interop on the mingw and msvc platform. Users of pread already have to take care that its not thread-safe on some platforms, now you're adding another breakage that has to be considered in future development.\n>>\n>> The mingw_pread implementation in [1] is both thread-safe and allows mixing read/pread in single-threaded scenarios, why not use this instead?\n>>\n>> [1] http://article.gmane.org/gmane.comp.version-control.git/242120\n> \n> \n> That's not thread-safe.  There is, presumably, a point between the\n> first and second calls to lseek64 when the implicit position pointer\n> is incorrect.  If another thread executes its first call to lseek64 at\n> that time, then the file descriptor may end up with an incorrect\n> position pointer after all threads have finished.\n> \n\nCorrect, a multi-threaded code section using pread has the effect of randomizing the file position (btw., this is also true for your pread implementation). This can be easily fixed by resetting the file position after pthread_join, if necessary. Currently there's just six callers of pthread_join.\n\n> Going forward, there is still a lot of performance that gets left on\n> the table when you rule out threaded file access.  There are not so\n> many calls to read, mmap, and pread in the code; it should be possible\n> to rationalize them and make them thread-safe -- at least, thread-safe\n> for posix-compliant systems and msysgit, which covers the great\n> majority of git users, I would hope.\n> \n\nIMO a \"mostly\" XSI compliant pread (or even the git_pread() emulation) is still better than forbidding the use of read() entirely. Switching from read to pread everywhere requires that all callers have to keep track of the file position, which means a _lot_ of code changes (read/xread/strbuf_read is used in ~70 places throughout git). And how do you plan to deal with platforms that don't have a thread-safe pread (HP, Cygwin)?\n\nConsidering all that, Duy's solution of opening separate file descriptors per thread seems to be the best pattern for future multi-threaded work.\n\nKarsten\n"},{"id":"237212","messageId":"CAHOQ7J-sUt3HGYNE7n=X3ZmV3Q-n+n9hMDAtzLbH3YU8iAqoqA@mail.gmail.com","threadId":"36214","inReplyTo":"532B5F0D.2070300@gmail.com","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Stefan Zager","fromEmail":"szager@chromium.org","sentAt":"2014-03-20T21:56:03Z","receivedAt":"2014-03-20T21:56:03Z","isPatch":true,"sender":{"key":"szager@chromium.org","avatar":null},"body":"On Thu, Mar 20, 2014 at 2:35 PM, Karsten Blees <karsten.blees@gmail.com> wrote:\n> Am 20.03.2014 17:08, schrieb Stefan Zager:\n>\n>> Going forward, there is still a lot of performance that gets left on\n>> the table when you rule out threaded file access.  There are not so\n>> many calls to read, mmap, and pread in the code; it should be possible\n>> to rationalize them and make them thread-safe -- at least, thread-safe\n>> for posix-compliant systems and msysgit, which covers the great\n>> majority of git users, I would hope.\n>>\n>\n> IMO a \"mostly\" XSI compliant pread (or even the git_pread() emulation) is still better than forbidding the use of read() entirely. Switching from read to pread everywhere requires that all callers have to keep track of the file position, which means a _lot_ of code changes (read/xread/strbuf_read is used in ~70 places throughout git). And how do you plan to deal with platforms that don't have a thread-safe pread (HP, Cygwin)?\n>\n> Considering all that, Duy's solution of opening separate file descriptors per thread seems to be the best pattern for future multi-threaded work.\n\nDoes that mean you would endorse the (N threads) * (M pack files)\napproach to threading checkout and status?  That seems kind of\ncrazy-town to me.  Not to mention that pack windows are not shared, so\nthis approach to multi-threading can have the side-effect of blowing\nout memory consumption.  We have already had to dial back settings for\npack.threads and core.deltaBaseCacheLimit, because threaded index-pack\nwas causing OOM errors on 32-bit platforms.\n\nCygwin (and MSVC) should be able to share a \"mostly\" compliant pread\nimplementation.  I don't have any insight into NonstopKernel; does is\nreally not have a thread-safe pread implementation?  If so, then I\nsuppose we have to #ifdef NO_PREAD, just as we do now.\n\nI realize that these are deep changes.  However, the performance of\nmsysgit on the chromium repositories is pretty awful, enough so to\nmotivate this work.\n\nStefan\n"},{"id":"237250","messageId":"CACsJy8A_CZYtHvVo5iMzgmP5t7XLxHNRD6n4D=_jiOLOvBgmWQ@mail.gmail.com","threadId":"36214","inReplyTo":"CAHOQ7J-sUt3HGYNE7n=X3ZmV3Q-n+n9hMDAtzLbH3YU8iAqoqA@mail.gmail.com","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-21T01:33:35Z","receivedAt":"2014-03-21T01:33:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 21, 2014 at 4:56 AM, Stefan Zager <szager@chromium.org> wrote:\n>> Considering all that, Duy's solution of opening separate file descriptors per thread seems to be the best pattern for future multi-threaded work.\n>\n> Does that mean you would endorse the (N threads) * (M pack files)\n> approach to threading checkout and status?  That seems kind of\n> crazy-town to me.  Not to mention that pack windows are not shared, so\n> this approach to multi-threading can have the side-effect of blowing\n> out memory consumption.\n\nMaybe we could protect and share the delta cache. Pack windows are\nmmap'd so we should not need to worry about their memory consumption.\n\n> We have already had to dial back settings for\n> pack.threads and core.deltaBaseCacheLimit, because threaded index-pack\n> was causing OOM errors on 32-bit platforms.\n\nHm.. I don't think index-pack uses sha1_file.c heavily. Local pack\naccess is only needed for verifying identical objects (and that should\nnever happen often). Something is fishy with these OOM errors.\n\n> Cygwin (and MSVC) should be able to share a \"mostly\" compliant pread\n> implementation.  I don't have any insight into NonstopKernel; does is\n> really not have a thread-safe pread implementation?  If so, then I\n> suppose we have to #ifdef NO_PREAD, just as we do now.\n>\n> I realize that these are deep changes.  However, the performance of\n> msysgit on the chromium repositories is pretty awful, enough so to\n> motivate this work.\n-- \nDuy\n"},{"id":"237251","messageId":"CACsJy8AYs0-rmGZz2_KEkT2ibW-sTpm=Q9FxFhNGRYd2b6R+sA@mail.gmail.com","threadId":"36214","inReplyTo":"CAHOQ7J9drXwcTt4b0Tcyw97KTGcifwsO5rtFNQYf7CVr3WD7zQ@mail.gmail.com","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-21T01:51:18Z","receivedAt":"2014-03-21T01:51:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 20, 2014 at 11:08 PM, Stefan Zager <szager@chromium.org> wrote:\n> Duy, would you like to re-post your patch without the new pread implementation?\n\nI will but let me try out the sliding window idea first. My quick\ntests on git.git show me we may only need 21k mmap instead of 177k\npread. That hints some potential performance improvement.\n-- \nDuy\n"},{"id":"237262","messageId":"20140321052118.GA28519@duynguyen-vnpc.dek-tpc.internal","threadId":"36214","inReplyTo":"CACsJy8AYs0-rmGZz2_KEkT2ibW-sTpm=Q9FxFhNGRYd2b6R+sA@mail.gmail.com","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-21T05:21:19Z","receivedAt":"2014-03-21T05:21:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 21, 2014 at 08:51:18AM +0700, Duy Nguyen wrote:\n> On Thu, Mar 20, 2014 at 11:08 PM, Stefan Zager <szager@chromium.org> wrote:\n> > Duy, would you like to re-post your patch without the new pread implementation?\n> \n> I will but let me try out the sliding window idea first. My quick\n> tests on git.git show me we may only need 21k mmap instead of 177k\n> pread. That hints some potential performance improvement.\n\nThe patch at the bottom reuses (un)use_pack() instead of pread(). The\nresults on linux-2.6 do not look any different. I guess we can drop\nthe idea.\n\nIt makes me wonder, though, what's wrong a simple patch like this to\nmake pread in index-pack thread-safe? It does not look any different\neither from the performance point of view, perhaps because\nunpack_data() reads small deltas most of the time\n\n-- 8< --\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex a6b1c17..b91f4f8 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -40,11 +40,6 @@ struct base_data {\n \tint ofs_first, ofs_last;\n };\n \n-#if !defined(NO_PTHREADS) && defined(NO_THREAD_SAFE_PREAD)\n-/* pread() emulation is not thread-safe. Disable threading. */\n-#define NO_PTHREADS\n-#endif\n-\n struct thread_local {\n #ifndef NO_PTHREADS\n \tpthread_t thread;\n@@ -175,6 +170,22 @@ static void cleanup_thread(void)\n #endif\n \n \n+#if defined(NO_THREAD_SAFE_PREAD)\n+static inline ssize_t pread_safe(int fd, void *buf, size_t count, off_t off)\n+{\n+\tint ret;\n+\tread_lock();\n+\tret = pread(fd, buf, count, off);\n+\tread_unlock();\n+\treturn ret;\n+}\n+#else\n+static inline ssize_t pread_safe(int fd, void *buf, size_t count, off_t off)\n+{\n+\treturn pread(fd, buf, count, off);\n+}\n+#endif\n+\n static int mark_link(struct object *obj, int type, void *data)\n {\n \tif (!obj)\n@@ -542,7 +553,7 @@ static void *unpack_data(struct object_entry *obj,\n \n \tdo {\n \t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n-\t\tn = pread(pack_fd, inbuf, n, from);\n+\t\tn = pread_safe(pack_fd, inbuf, n, from);\n \t\tif (n < 0)\n \t\t\tdie_errno(_(\"cannot pread pack file\"));\n \t\tif (!n)\n-- 8< --\n\nAnd the sliding window patch for the list archive\n\n-- 8< --\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex a6b1c17..6f5c6d9 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -91,7 +91,8 @@ static off_t consumed_bytes;\n static unsigned deepest_delta;\n static git_SHA_CTX input_ctx;\n static uint32_t input_crc32;\n-static int input_fd, output_fd, pack_fd;\n+static int input_fd, output_fd;\n+static struct packed_git *pack;\n \n #ifndef NO_PTHREADS\n \n@@ -224,8 +225,10 @@ static unsigned check_objects(void)\n static void flush(void)\n {\n \tif (input_offset) {\n-\t\tif (output_fd >= 0)\n+\t\tif (output_fd >= 0) {\n \t\t\twrite_or_die(output_fd, input_buffer, input_offset);\n+\t\t\tpack->pack_size += input_offset;\n+\t\t}\n \t\tgit_SHA1_Update(&input_ctx, input_buffer, input_offset);\n \t\tmemmove(input_buffer, input_buffer + input_offset, input_len);\n \t\tinput_offset = 0;\n@@ -277,6 +280,10 @@ static void use(int bytes)\n \n static const char *open_pack_file(const char *pack_name)\n {\n+\tpack = xmalloc(sizeof(*pack) + 1);\n+\tmemset(pack, 0, sizeof(*pack));\n+\tpack->pack_name[0] = '\\0';\n+\n \tif (from_stdin) {\n \t\tinput_fd = 0;\n \t\tif (!pack_name) {\n@@ -288,13 +295,17 @@ static const char *open_pack_file(const char *pack_name)\n \t\t\toutput_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);\n \t\tif (output_fd < 0)\n \t\t\tdie_errno(_(\"unable to create '%s'\"), pack_name);\n-\t\tpack_fd = output_fd;\n+\t\tpack->pack_fd = output_fd;\n \t} else {\n+\t\tstruct stat st;\n \t\tinput_fd = open(pack_name, O_RDONLY);\n \t\tif (input_fd < 0)\n \t\t\tdie_errno(_(\"cannot open packfile '%s'\"), pack_name);\n+\t\tif (lstat(pack_name, &st))\n+\t\t\tdie_errno(_(\"cannot stat packfile '%s'\"), pack_name);\n \t\toutput_fd = -1;\n-\t\tpack_fd = input_fd;\n+\t\tpack->pack_fd = input_fd;\n+\t\tpack->pack_size = st.st_size;\n \t}\n \tgit_SHA1_Init(&input_ctx);\n \treturn pack_name;\n@@ -531,9 +542,15 @@ static void *unpack_data(struct object_entry *obj,\n \tunsigned char *data, *inbuf;\n \tgit_zstream stream;\n \tint status;\n+\tstruct pack_window *w_cursor = NULL;\n+\n+\tif (from + len > pack->pack_size)\n+\t\tdie(Q_(\"premature end of pack file, %lu byte missing\",\n+\t\t       \"premature end of pack file, %lu bytes missing\",\n+\t\t       from + len - pack->pack_size),\n+\t\t    (unsigned long)(from + len - pack->pack_size));\n \n \tdata = xmalloc(consume ? 64*1024 : obj->size);\n-\tinbuf = xmalloc((len < 64*1024) ? len : 64*1024);\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init(&stream);\n@@ -541,15 +558,12 @@ static void *unpack_data(struct object_entry *obj,\n \tstream.avail_out = consume ? 64*1024 : obj->size;\n \n \tdo {\n-\t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n-\t\tn = pread(pack_fd, inbuf, n, from);\n-\t\tif (n < 0)\n-\t\t\tdie_errno(_(\"cannot pread pack file\"));\n-\t\tif (!n)\n-\t\t\tdie(Q_(\"premature end of pack file, %lu byte missing\",\n-\t\t\t       \"premature end of pack file, %lu bytes missing\",\n-\t\t\t       len),\n-\t\t\t    len);\n+\t\tssize_t n;\n+\t\tunsigned long left;\n+\t\tread_lock();\n+\t\tinbuf = use_pack(pack, &w_cursor, from, &left);\n+\t\tread_unlock();\n+\t\tn = left > len ? len : left;\n \t\tfrom += n;\n \t\tlen -= n;\n \t\tstream.next_in = inbuf;\n@@ -568,6 +582,9 @@ static void *unpack_data(struct object_entry *obj,\n \t\t\t\tstream.avail_out = 64*1024;\n \t\t\t} while (status == Z_OK && stream.avail_in);\n \t\t}\n+\t\tread_lock();\n+\t\tunuse_pack(&w_cursor);\n+\t\tread_unlock();\n \t} while (len && status == Z_OK && !stream.avail_in);\n \n \t/* This has been inflated OK when first encountered, so... */\n@@ -575,7 +592,6 @@ static void *unpack_data(struct object_entry *obj,\n \t\tdie(_(\"serious inflate inconsistency\"));\n \n \tgit_inflate_end(&stream);\n-\tfree(inbuf);\n \tif (consume) {\n \t\tfree(data);\n \t\tdata = NULL;\n@@ -1657,6 +1673,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tfree(objects);\n \tfree(index_name_buf);\n \tfree(keep_name_buf);\n+\tclose_pack_windows(pack);\n+\tfree(pack);\n \tif (pack_name == NULL)\n \t\tfree((void *) curr_pack);\n \tif (index_name == NULL)\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 187f5a6..aa0b16d 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -977,7 +977,13 @@ unsigned char *use_pack(struct packed_git *p,\n \t */\n \tif (!p->pack_size && p->pack_fd == -1 && open_packed_git(p))\n \t\tdie(\"packfile %s cannot be accessed\", p->pack_name);\n-\tif (offset > (p->pack_size - 20))\n+\t/*\n+\t * index-pack uses this function even if the pack is not\n+\t * complete yet (i.e. trailing SHA-1 missing). Loosen the\n+\t * check a bit in this case (pack_empty name uses as the\n+\t * indicator).\n+\t */\n+\tif (offset > (p->pack_size - (*p->pack_name ? 20 : 0)))\n \t\tdie(\"offset beyond end of packfile (truncated pack?)\");\n \n \tif (!win || !in_window(win, offset)) {\n-- 8< --\n\n--\nDuy\n"},{"id":"237264","messageId":"CAHOQ7J8eEUd+NpL78RQqGFYzhD9Fs0hdGOHhmXiujJdGrfeS=A@mail.gmail.com","threadId":"36214","inReplyTo":"20140321052118.GA28519@duynguyen-vnpc.dek-tpc.internal","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Stefan Zager","fromEmail":"szager@chromium.org","sentAt":"2014-03-21T05:35:52Z","receivedAt":"2014-03-21T05:35:52Z","isPatch":true,"sender":{"key":"szager@chromium.org","avatar":null},"body":"On Thu, Mar 20, 2014 at 10:21 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n> On Fri, Mar 21, 2014 at 08:51:18AM +0700, Duy Nguyen wrote:\n>> On Thu, Mar 20, 2014 at 11:08 PM, Stefan Zager <szager@chromium.org> wrote:\n>> > Duy, would you like to re-post your patch without the new pread implementation?\n>>\n>> I will but let me try out the sliding window idea first. My quick\n>> tests on git.git show me we may only need 21k mmap instead of 177k\n>> pread. That hints some potential performance improvement.\n>\n> The patch at the bottom reuses (un)use_pack() instead of pread(). The\n> results on linux-2.6 do not look any different. I guess we can drop\n> the idea.\n>\n> It makes me wonder, though, what's wrong a simple patch like this to\n> make pread in index-pack thread-safe? It does not look any different\n> either from the performance point of view, perhaps because\n> unpack_data() reads small deltas most of the time\n\nWhen you serialize disk access in this way, the effect on performance\nis really dependent on the behavior of the OS, as well as the locality\nof the read offsets.  Assuming -- fairly, I think -- that the reads\nwill be pretty randomly distributed (i.e., no locality to speak of),\nthen your best bet is get as many read operations in flight as\npossible, and let the disk scheduler optimize the seek time.\n\nIf I have a chance, I will try out this patch in msysgit on the\nchromium repositories.\n\n>\n> -- 8< --\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index a6b1c17..b91f4f8 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -40,11 +40,6 @@ struct base_data {\n>         int ofs_first, ofs_last;\n>  };\n>\n> -#if !defined(NO_PTHREADS) && defined(NO_THREAD_SAFE_PREAD)\n> -/* pread() emulation is not thread-safe. Disable threading. */\n> -#define NO_PTHREADS\n> -#endif\n> -\n>  struct thread_local {\n>  #ifndef NO_PTHREADS\n>         pthread_t thread;\n> @@ -175,6 +170,22 @@ static void cleanup_thread(void)\n>  #endif\n>\n>\n> +#if defined(NO_THREAD_SAFE_PREAD)\n> +static inline ssize_t pread_safe(int fd, void *buf, size_t count, off_t off)\n> +{\n> +       int ret;\n> +       read_lock();\n> +       ret = pread(fd, buf, count, off);\n> +       read_unlock();\n> +       return ret;\n> +}\n> +#else\n> +static inline ssize_t pread_safe(int fd, void *buf, size_t count, off_t off)\n> +{\n> +       return pread(fd, buf, count, off);\n> +}\n> +#endif\n> +\n>  static int mark_link(struct object *obj, int type, void *data)\n>  {\n>         if (!obj)\n> @@ -542,7 +553,7 @@ static void *unpack_data(struct object_entry *obj,\n>\n>         do {\n>                 ssize_t n = (len < 64*1024) ? len : 64*1024;\n> -               n = pread(pack_fd, inbuf, n, from);\n> +               n = pread_safe(pack_fd, inbuf, n, from);\n>                 if (n < 0)\n>                         die_errno(_(\"cannot pread pack file\"));\n>                 if (!n)\n> -- 8< --\n>\n> And the sliding window patch for the list archive\n>\n> -- 8< --\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index a6b1c17..6f5c6d9 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -91,7 +91,8 @@ static off_t consumed_bytes;\n>  static unsigned deepest_delta;\n>  static git_SHA_CTX input_ctx;\n>  static uint32_t input_crc32;\n> -static int input_fd, output_fd, pack_fd;\n> +static int input_fd, output_fd;\n> +static struct packed_git *pack;\n>\n>  #ifndef NO_PTHREADS\n>\n> @@ -224,8 +225,10 @@ static unsigned check_objects(void)\n>  static void flush(void)\n>  {\n>         if (input_offset) {\n> -               if (output_fd >= 0)\n> +               if (output_fd >= 0) {\n>                         write_or_die(output_fd, input_buffer, input_offset);\n> +                       pack->pack_size += input_offset;\n> +               }\n>                 git_SHA1_Update(&input_ctx, input_buffer, input_offset);\n>                 memmove(input_buffer, input_buffer + input_offset, input_len);\n>                 input_offset = 0;\n> @@ -277,6 +280,10 @@ static void use(int bytes)\n>\n>  static const char *open_pack_file(const char *pack_name)\n>  {\n> +       pack = xmalloc(sizeof(*pack) + 1);\n> +       memset(pack, 0, sizeof(*pack));\n> +       pack->pack_name[0] = '\\0';\n> +\n>         if (from_stdin) {\n>                 input_fd = 0;\n>                 if (!pack_name) {\n> @@ -288,13 +295,17 @@ static const char *open_pack_file(const char *pack_name)\n>                         output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);\n>                 if (output_fd < 0)\n>                         die_errno(_(\"unable to create '%s'\"), pack_name);\n> -               pack_fd = output_fd;\n> +               pack->pack_fd = output_fd;\n>         } else {\n> +               struct stat st;\n>                 input_fd = open(pack_name, O_RDONLY);\n>                 if (input_fd < 0)\n>                         die_errno(_(\"cannot open packfile '%s'\"), pack_name);\n> +               if (lstat(pack_name, &st))\n> +                       die_errno(_(\"cannot stat packfile '%s'\"), pack_name);\n>                 output_fd = -1;\n> -               pack_fd = input_fd;\n> +               pack->pack_fd = input_fd;\n> +               pack->pack_size = st.st_size;\n>         }\n>         git_SHA1_Init(&input_ctx);\n>         return pack_name;\n> @@ -531,9 +542,15 @@ static void *unpack_data(struct object_entry *obj,\n>         unsigned char *data, *inbuf;\n>         git_zstream stream;\n>         int status;\n> +       struct pack_window *w_cursor = NULL;\n> +\n> +       if (from + len > pack->pack_size)\n> +               die(Q_(\"premature end of pack file, %lu byte missing\",\n> +                      \"premature end of pack file, %lu bytes missing\",\n> +                      from + len - pack->pack_size),\n> +                   (unsigned long)(from + len - pack->pack_size));\n>\n>         data = xmalloc(consume ? 64*1024 : obj->size);\n> -       inbuf = xmalloc((len < 64*1024) ? len : 64*1024);\n>\n>         memset(&stream, 0, sizeof(stream));\n>         git_inflate_init(&stream);\n> @@ -541,15 +558,12 @@ static void *unpack_data(struct object_entry *obj,\n>         stream.avail_out = consume ? 64*1024 : obj->size;\n>\n>         do {\n> -               ssize_t n = (len < 64*1024) ? len : 64*1024;\n> -               n = pread(pack_fd, inbuf, n, from);\n> -               if (n < 0)\n> -                       die_errno(_(\"cannot pread pack file\"));\n> -               if (!n)\n> -                       die(Q_(\"premature end of pack file, %lu byte missing\",\n> -                              \"premature end of pack file, %lu bytes missing\",\n> -                              len),\n> -                           len);\n> +               ssize_t n;\n> +               unsigned long left;\n> +               read_lock();\n> +               inbuf = use_pack(pack, &w_cursor, from, &left);\n> +               read_unlock();\n> +               n = left > len ? len : left;\n>                 from += n;\n>                 len -= n;\n>                 stream.next_in = inbuf;\n> @@ -568,6 +582,9 @@ static void *unpack_data(struct object_entry *obj,\n>                                 stream.avail_out = 64*1024;\n>                         } while (status == Z_OK && stream.avail_in);\n>                 }\n> +               read_lock();\n> +               unuse_pack(&w_cursor);\n> +               read_unlock();\n>         } while (len && status == Z_OK && !stream.avail_in);\n>\n>         /* This has been inflated OK when first encountered, so... */\n> @@ -575,7 +592,6 @@ static void *unpack_data(struct object_entry *obj,\n>                 die(_(\"serious inflate inconsistency\"));\n>\n>         git_inflate_end(&stream);\n> -       free(inbuf);\n>         if (consume) {\n>                 free(data);\n>                 data = NULL;\n> @@ -1657,6 +1673,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n>         free(objects);\n>         free(index_name_buf);\n>         free(keep_name_buf);\n> +       close_pack_windows(pack);\n> +       free(pack);\n>         if (pack_name == NULL)\n>                 free((void *) curr_pack);\n>         if (index_name == NULL)\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 187f5a6..aa0b16d 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -977,7 +977,13 @@ unsigned char *use_pack(struct packed_git *p,\n>          */\n>         if (!p->pack_size && p->pack_fd == -1 && open_packed_git(p))\n>                 die(\"packfile %s cannot be accessed\", p->pack_name);\n> -       if (offset > (p->pack_size - 20))\n> +       /*\n> +        * index-pack uses this function even if the pack is not\n> +        * complete yet (i.e. trailing SHA-1 missing). Loosen the\n> +        * check a bit in this case (pack_empty name uses as the\n> +        * indicator).\n> +        */\n> +       if (offset > (p->pack_size - (*p->pack_name ? 20 : 0)))\n>                 die(\"offset beyond end of packfile (truncated pack?)\");\n>\n>         if (!win || !in_window(win, offset)) {\n> -- 8< --\n>\n> --\n> Duy\n"},{"id":"237347","messageId":"532C8B0A.70202@gmail.com","threadId":"36214","inReplyTo":"CAHOQ7J8eEUd+NpL78RQqGFYzhD9Fs0hdGOHhmXiujJdGrfeS=A@mail.gmail.com","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-03-21T18:55:06Z","receivedAt":"2014-03-21T18:55:06Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 21.03.2014 06:35, schrieb Stefan Zager:\n> On Thu, Mar 20, 2014 at 10:21 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n>> On Fri, Mar 21, 2014 at 08:51:18AM +0700, Duy Nguyen wrote:\n>>> On Thu, Mar 20, 2014 at 11:08 PM, Stefan Zager <szager@chromium.org> wrote:\n>>>> Duy, would you like to re-post your patch without the new pread implementation?\n>>>\n>>> I will but let me try out the sliding window idea first. My quick\n>>> tests on git.git show me we may only need 21k mmap instead of 177k\n>>> pread. That hints some potential performance improvement.\n>>\n>> The patch at the bottom reuses (un)use_pack() instead of pread(). The\n>> results on linux-2.6 do not look any different. I guess we can drop\n>> the idea.\n>>\n>> It makes me wonder, though, what's wrong a simple patch like this to\n>> make pread in index-pack thread-safe? It does not look any different\n>> either from the performance point of view, perhaps because\n>> unpack_data() reads small deltas most of the time\n> \n> When you serialize disk access in this way, the effect on performance\n> is really dependent on the behavior of the OS, as well as the locality\n> of the read offsets.  Assuming -- fairly, I think -- that the reads\n> will be pretty randomly distributed (i.e., no locality to speak of),\n> then your best bet is get as many read operations in flight as\n> possible, and let the disk scheduler optimize the seek time.\n> \n\nThe read() implementation in MSVCRT.DLL is synchronized anyway, and I strongly suspect that this is also true for ReadFile() (at least for synchronous file handles, i.e. opened without FILE_FLAG_OVERLAPPED). So I guess separate file descriptors would help with parallel IO as well.\n"},{"id":"237354","messageId":"532C9AA9.1010102@gmail.com","threadId":"36214","inReplyTo":"CAHOQ7J-sUt3HGYNE7n=X3ZmV3Q-n+n9hMDAtzLbH3YU8iAqoqA@mail.gmail.com","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-03-21T20:01:45Z","receivedAt":"2014-03-21T20:01:45Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 20.03.2014 22:56, schrieb Stefan Zager:\n> On Thu, Mar 20, 2014 at 2:35 PM, Karsten Blees <karsten.blees@gmail.com> wrote:\n>> Am 20.03.2014 17:08, schrieb Stefan Zager:\n>>\n>>> Going forward, there is still a lot of performance that gets left on\n>>> the table when you rule out threaded file access.  There are not so\n>>> many calls to read, mmap, and pread in the code; it should be possible\n>>> to rationalize them and make them thread-safe -- at least, thread-safe\n>>> for posix-compliant systems and msysgit, which covers the great\n>>> majority of git users, I would hope.\n>>>\n>>\n>> IMO a \"mostly\" XSI compliant pread (or even the git_pread() emulation) is still better than forbidding the use of read() entirely. Switching from read to pread everywhere requires that all callers have to keep track of the file position, which means a _lot_ of code changes (read/xread/strbuf_read is used in ~70 places throughout git). And how do you plan to deal with platforms that don't have a thread-safe pread (HP, Cygwin)?\n>>\n>> Considering all that, Duy's solution of opening separate file descriptors per thread seems to be the best pattern for future multi-threaded work.\n> \n> Does that mean you would endorse the (N threads) * (M pack files)\n> approach to threading checkout and status?  That seems kind of\n> crazy-town to me.  Not to mention that pack windows are not shared, so\n> this approach to multi-threading can have the side-effect of blowing\n> out memory consumption.  We have already had to dial back settings for\n> pack.threads and core.deltaBaseCacheLimit, because threaded index-pack\n> was causing OOM errors on 32-bit platforms.\n> \n\nOpening more file descriptors doesn't significantly increase the memory footprint, so it shouldn't matter whether the threads read data via shared or private descriptors.\n\ngit-status with core.preloadindex is already multithreaded (at least the first part), and AFAIK doesn't read pack files at all.\n\nI'm still not convinced that multi-threaded git-checkout is a good idea. According to my tests this is actually slower than sequential checkout. You'd have to be very careful to only multi-thread the parts that don't do any IO, such as unpacking / undeltifying.\n"},{"id":"237655","messageId":"1395754901-19730-1-git-send-email-pclouds@gmail.com","threadId":"36214","inReplyTo":"CACsJy8AYs0-rmGZz2_KEkT2ibW-sTpm=Q9FxFhNGRYd2b6R+sA@mail.gmail.com","subject":"[PATCH] index-pack: work around thread-unsafe pread()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-25T13:41:41Z","receivedAt":"2014-03-25T13:41:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"pread() implementation for Cygwin and MSYS is not thread safe, which\nled to multithreading being disabled in c0f8654 (index-pack: Disable\nthreading on cygwin - 2012-06-26). Work around it by opening one file\nhandle per thread, so parallel pread() (on different file handle)\ncan't step on each other. Also remove NO_THREAD_SAFE_PREAD that was\nintroduced in c0f8654 because it's no longer used anywhere.\n\nThis workaround is unconditional, even for platforms with thread-safe\npread() because the overhead is small (a couple file handles more) and\nnot worth fragmenting the code.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n On Fri, Mar 21, 2014 at 8:51 AM, Duy Nguyen <pclouds@gmail.com> wrote:\n > On Thu, Mar 20, 2014 at 11:08 PM, Stefan Zager <szager@chromium.org> wrote:\n >> Duy, would you like to re-post your patch without the new pread implementation?\n >\n > I will but let me try out the sliding window idea first. My quick\n > tests on git.git show me we may only need 21k mmap instead of 177k\n > pread. That hints some potential performance improvement.\n\n Here it is. But I still think it's worth measuring the simpler patch\n that protects pread() from the caller. I suspect we won't see any\n difference.\n\n Makefile             |  7 -------\n builtin/index-pack.c | 27 +++++++++++++++++----------\n config.mak.uname     |  1 -\n 3 files changed, 17 insertions(+), 18 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 3646391..0089fad 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -183,9 +183,6 @@ all::\n # Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval\n # This also implies NO_SETITIMER\n #\n-# Define NO_THREAD_SAFE_PREAD if your pread() implementation is not\n-# thread-safe. (e.g. compat/pread.c or cygwin)\n-#\n # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is\n # generally faster on your platform than accessing the working directory.\n #\n@@ -1336,10 +1333,6 @@ endif\n ifdef NO_PREAD\n \tCOMPAT_CFLAGS += -DNO_PREAD\n \tCOMPAT_OBJS += compat/pread.o\n-\tNO_THREAD_SAFE_PREAD = YesPlease\n-endif\n-ifdef NO_THREAD_SAFE_PREAD\n-\tBASIC_CFLAGS += -DNO_THREAD_SAFE_PREAD\n endif\n ifdef NO_FAST_WORKING_DIRECTORY\n \tBASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex a6b1c17..676d39d 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -40,17 +40,13 @@ struct base_data {\n \tint ofs_first, ofs_last;\n };\n \n-#if !defined(NO_PTHREADS) && defined(NO_THREAD_SAFE_PREAD)\n-/* pread() emulation is not thread-safe. Disable threading. */\n-#define NO_PTHREADS\n-#endif\n-\n struct thread_local {\n #ifndef NO_PTHREADS\n \tpthread_t thread;\n #endif\n \tstruct base_data *base_cache;\n \tsize_t base_cache_used;\n+\tint pack_fd;\n };\n \n /*\n@@ -91,7 +87,8 @@ static off_t consumed_bytes;\n static unsigned deepest_delta;\n static git_SHA_CTX input_ctx;\n static uint32_t input_crc32;\n-static int input_fd, output_fd, pack_fd;\n+static int input_fd, output_fd;\n+static const char *curr_pack;\n \n #ifndef NO_PTHREADS\n \n@@ -134,6 +131,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)\n  */\n static void init_thread(void)\n {\n+\tint i;\n \tinit_recursive_mutex(&read_mutex);\n \tpthread_mutex_init(&counter_mutex, NULL);\n \tpthread_mutex_init(&work_mutex, NULL);\n@@ -141,11 +139,18 @@ static void init_thread(void)\n \t\tpthread_mutex_init(&deepest_delta_mutex, NULL);\n \tpthread_key_create(&key, NULL);\n \tthread_data = xcalloc(nr_threads, sizeof(*thread_data));\n+\tfor (i = 0; i < nr_threads; i++) {\n+\t\tthread_data[i].pack_fd = open(curr_pack, O_RDONLY);\n+\t\tif (thread_data[i].pack_fd == -1)\n+\t\t\tdie_errno(_(\"unable to open %s\"), curr_pack);\n+\t}\n+\n \tthreads_active = 1;\n }\n \n static void cleanup_thread(void)\n {\n+\tint i;\n \tif (!threads_active)\n \t\treturn;\n \tthreads_active = 0;\n@@ -154,6 +159,8 @@ static void cleanup_thread(void)\n \tpthread_mutex_destroy(&work_mutex);\n \tif (show_stat)\n \t\tpthread_mutex_destroy(&deepest_delta_mutex);\n+\tfor (i = 0; i < nr_threads; i++)\n+\t\tclose(thread_data[i].pack_fd);\n \tpthread_key_delete(key);\n \tfree(thread_data);\n }\n@@ -288,13 +295,13 @@ static const char *open_pack_file(const char *pack_name)\n \t\t\toutput_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);\n \t\tif (output_fd < 0)\n \t\t\tdie_errno(_(\"unable to create '%s'\"), pack_name);\n-\t\tpack_fd = output_fd;\n+\t\tnothread_data.pack_fd = output_fd;\n \t} else {\n \t\tinput_fd = open(pack_name, O_RDONLY);\n \t\tif (input_fd < 0)\n \t\t\tdie_errno(_(\"cannot open packfile '%s'\"), pack_name);\n \t\toutput_fd = -1;\n-\t\tpack_fd = input_fd;\n+\t\tnothread_data.pack_fd = input_fd;\n \t}\n \tgit_SHA1_Init(&input_ctx);\n \treturn pack_name;\n@@ -542,7 +549,7 @@ static void *unpack_data(struct object_entry *obj,\n \n \tdo {\n \t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n-\t\tn = pread(pack_fd, inbuf, n, from);\n+\t\tn = pread(get_thread_data()->pack_fd, inbuf, n, from);\n \t\tif (n < 0)\n \t\t\tdie_errno(_(\"cannot pread pack file\"));\n \t\tif (!n)\n@@ -1490,7 +1497,7 @@ static void show_pack_info(int stat_only)\n int cmd_index_pack(int argc, const char **argv, const char *prefix)\n {\n \tint i, fix_thin_pack = 0, verify = 0, stat_only = 0;\n-\tconst char *curr_pack, *curr_index;\n+\tconst char *curr_index;\n \tconst char *index_name = NULL, *pack_name = NULL;\n \tconst char *keep_name = NULL, *keep_msg = NULL;\n \tchar *index_name_buf = NULL, *keep_name_buf = NULL;\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 6069a44..c9febe1 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -157,7 +157,6 @@ ifeq ($(uname_O),Cygwin)\n \t\tNO_SYMLINK_HEAD = YesPlease\n \t\tNO_IPV6 = YesPlease\n \t\tOLD_ICONV = UnfortunatelyYes\n-\t\tNO_THREAD_SAFE_PREAD = YesPlease\n \t\t# There are conflicting reports about this.\n \t\t# On some boxes NO_MMAP is needed, and not so elsewhere.\n \t\t# Try commenting this out if you suspect MMAP is more efficient\n-- \n1.9.1.345.ga1a145c\n"},{"id":"237791","messageId":"5332914A.7030407@viscovery.net","threadId":"36214","inReplyTo":"5328e903.joAd1dfenJmScBNr%szager@chromium.org","subject":"Re: [PATCH] Enable index-pack threading in msysgit.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-03-26T08:35:22Z","receivedAt":"2014-03-26T08:35:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/19/2014 1:46, schrieb szager@chromium.org:\n> This adds a Windows implementation of pread.  Note that it is NOT\n> safe to intersperse calls to read() and pread() on a file\n> descriptor.  According to the ReadFile spec, using the 'overlapped'\n> argument should not affect the implicit position pointer of the\n> descriptor.  Experiments have shown that this is, in fact, a lie.\n> \n> To accomodate that fact, this change also incorporates:\n> \n> http://article.gmane.org/gmane.comp.version-control.git/196042\n> \n> .... which gives each index-pack thread its own file descriptor.\n> ---\n>  builtin/index-pack.c | 21 ++++++++++++++++-----\n>  compat/mingw.c       | 31 ++++++++++++++++++++++++++++++-\n>  compat/mingw.h       |  3 +++\n>  config.mak.uname     |  1 -\n>  4 files changed, 49 insertions(+), 7 deletions(-)\n\nt5302 does not pass with this patch (sz/mingw-index-pack-threaded).\nIt fails like this:\n\n+ eval 'git index-pack --index-version=1 --stdin < \"test-1-${pack1}.pack\" &&\n     git prune-packed &&\n     git count-objects | ( read nr rest && test \"$nr\" -eq 1 ) &&\n     cmp \"test-1-${pack1}.pack\" \".git/objects/pack/pack-${pack1}.pack\" &&\n     cmp \"test-1-${pack1}.idx\"  \".git/objects/pack/pack-${pack1}.idx\"'\n++ git index-pack --index-version=1 --stdin\npack\t1c54d893dd9bf6645ecee2886ea72f2c2030bea1\n++ git prune-packed\nerror: packfile .git/objects/pack/pack-1c54d893dd9bf6645ecee2886ea72f2c2030bea1.pack does not match index\nwarning: packfile .git/objects/pack/pack-1c54d893dd9bf6645ecee2886ea72f2c2030bea1.pack cannot be accessed\n[... these 2 messages repeat ~250 times ...]\n++ git count-objects\n++ read nr rest\n++ test 303 -eq 1\n\nI haven't tested Duy's latest patch (index-pack: work around\nthread-unsafe pread() yesterday), yet.\n\n-- Hannes\n\n> \n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 2f37a38..c02dd4c 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -51,6 +51,7 @@ struct thread_local {\n>  #endif\n>  \tstruct base_data *base_cache;\n>  \tsize_t base_cache_used;\n> +\tint pack_fd;\n>  };\n>  \n>  /*\n> @@ -91,7 +92,8 @@ static off_t consumed_bytes;\n>  static unsigned deepest_delta;\n>  static git_SHA_CTX input_ctx;\n>  static uint32_t input_crc32;\n> -static int input_fd, output_fd, pack_fd;\n> +static const char *curr_pack;\n> +static int input_fd, output_fd;\n>  \n>  #ifndef NO_PTHREADS\n>  \n> @@ -134,6 +136,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)\n>   */\n>  static void init_thread(void)\n>  {\n> +\tint i;\n>  \tinit_recursive_mutex(&read_mutex);\n>  \tpthread_mutex_init(&counter_mutex, NULL);\n>  \tpthread_mutex_init(&work_mutex, NULL);\n> @@ -141,11 +144,17 @@ static void init_thread(void)\n>  \t\tpthread_mutex_init(&deepest_delta_mutex, NULL);\n>  \tpthread_key_create(&key, NULL);\n>  \tthread_data = xcalloc(nr_threads, sizeof(*thread_data));\n> +\tfor (i = 0; i < nr_threads; i++) {\n> +\t\tthread_data[i].pack_fd = open(curr_pack, O_RDONLY);\n> +\t\tif (thread_data[i].pack_fd == -1)\n> +\t\t\tdie_errno(\"unable to open %s\", curr_pack);\n> +\t}\n>  \tthreads_active = 1;\n>  }\n>  \n>  static void cleanup_thread(void)\n>  {\n> +\tint i;\n>  \tif (!threads_active)\n>  \t\treturn;\n>  \tthreads_active = 0;\n> @@ -155,6 +164,8 @@ static void cleanup_thread(void)\n>  \tif (show_stat)\n>  \t\tpthread_mutex_destroy(&deepest_delta_mutex);\n>  \tpthread_key_delete(key);\n> +\tfor (i = 0; i < nr_threads; i++)\n> +\t\tclose(thread_data[i].pack_fd);\n>  \tfree(thread_data);\n>  }\n>  \n> @@ -288,13 +299,13 @@ static const char *open_pack_file(const char *pack_name)\n>  \t\t\toutput_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);\n>  \t\tif (output_fd < 0)\n>  \t\t\tdie_errno(_(\"unable to create '%s'\"), pack_name);\n> -\t\tpack_fd = output_fd;\n> +\t\tnothread_data.pack_fd = output_fd;\n>  \t} else {\n>  \t\tinput_fd = open(pack_name, O_RDONLY);\n>  \t\tif (input_fd < 0)\n>  \t\t\tdie_errno(_(\"cannot open packfile '%s'\"), pack_name);\n>  \t\toutput_fd = -1;\n> -\t\tpack_fd = input_fd;\n> +\t\tnothread_data.pack_fd = input_fd;\n>  \t}\n>  \tgit_SHA1_Init(&input_ctx);\n>  \treturn pack_name;\n> @@ -542,7 +553,7 @@ static void *unpack_data(struct object_entry *obj,\n>  \n>  \tdo {\n>  \t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n> -\t\tn = pread(pack_fd, inbuf, n, from);\n> +\t\tn = pread(get_thread_data()->pack_fd, inbuf, n, from);\n>  \t\tif (n < 0)\n>  \t\t\tdie_errno(_(\"cannot pread pack file\"));\n>  \t\tif (!n)\n> @@ -1490,7 +1501,7 @@ static void show_pack_info(int stat_only)\n>  int cmd_index_pack(int argc, const char **argv, const char *prefix)\n>  {\n>  \tint i, fix_thin_pack = 0, verify = 0, stat_only = 0;\n> -\tconst char *curr_pack, *curr_index;\n> +\tconst char *curr_index;\n>  \tconst char *index_name = NULL, *pack_name = NULL;\n>  \tconst char *keep_name = NULL, *keep_msg = NULL;\n>  \tchar *index_name_buf = NULL, *keep_name_buf = NULL;\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 383cafe..6cc85d6 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -329,7 +329,36 @@ int mingw_mkdir(const char *path, int mode)\n>  \treturn ret;\n>  }\n>  \n> -int mingw_open (const char *filename, int oflags, ...)\n> +\n> +ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)\n> +{\n> +\tHANDLE hand = (HANDLE)_get_osfhandle(fd);\n> +\tif (hand == INVALID_HANDLE_VALUE) {\n> +\t\terrno = EBADF;\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tLARGE_INTEGER offset_value;\n> +\toffset_value.QuadPart = offset;\n> +\n> +\tDWORD bytes_read = 0;\n> +\tOVERLAPPED overlapped = {0};\n> +\toverlapped.Offset = offset_value.LowPart;\n> +\toverlapped.OffsetHigh = offset_value.HighPart;\n> +\tBOOL result = ReadFile(hand, buf, count, &bytes_read, &overlapped);\n> +\n> +\tssize_t ret = bytes_read;\n> +\n> +\tif (!result && GetLastError() != ERROR_HANDLE_EOF)\n> +\t{\n> +\t\terrno = err_win_to_posix(GetLastError());\n> +\t\tret = -1;\n> +\t}\n> +\n> +\treturn ret;\n> +}\n> +\n> +int mingw_open(const char *filename, int oflags, ...)\n>  {\n>  \tva_list args;\n>  \tunsigned mode;\n> diff --git a/compat/mingw.h b/compat/mingw.h\n> index 08b83fe..377ba50 100644\n> --- a/compat/mingw.h\n> +++ b/compat/mingw.h\n> @@ -174,6 +174,9 @@ int mingw_unlink(const char *pathname);\n>  int mingw_rmdir(const char *path);\n>  #define rmdir mingw_rmdir\n>  \n> +ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset);\n> +#define pread mingw_pread\n> +\n>  int mingw_open (const char *filename, int oflags, ...);\n>  #define open mingw_open\n>  \n> diff --git a/config.mak.uname b/config.mak.uname\n> index e8acc39..b405524 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -474,7 +474,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n>  endif\n>  ifneq (,$(findstring MINGW,$(uname_S)))\n>  \tpathsep = ;\n> -\tNO_PREAD = YesPlease\n>  \tNEEDS_CRYPTO_WITH_SSL = YesPlease\n>  \tNO_LIBGEN_H = YesPlease\n>  \tNO_POLL = YesPlease\n> \n"}]}