Re: What's cooking in git.git (Apr 2012, #06; Sun, 15)

[Erik Faye-Lund <kusmabite@gmail.com>]

On Thu, Apr 19, 2012 at 11:36 AM, Nguyen Thai Ngoc Duy
<pclouds@gmail.com> wrote:
> On Thu, Apr 19, 2012 at 08:45:52AM +0200, Johannes Sixt wrote:
>> I don't see any mutual exclusion happening in this chain. Perhaps it is
>> not needed, provided that the pread() call in get_data_from_pack is
>> atomic. But our git_pread() from compat/pread.c, which we use on Windows,
>> is not atomic.
>
> OK assume it's Windows' pread thread-safe problem (it does look like
> so given how pread.c implements it), maybe this patch will help:
>
> -- 8< --
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 847dbb3..5d539cd 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -102,6 +102,10 @@ static pthread_mutex_t work_mutex;
>  #define work_lock()            lock_mutex(&work_mutex)
>  #define work_unlock()          unlock_mutex(&work_mutex)
>
> +#ifdef WIN32
> +static pthread_mutex_t pread_mutex;
> +#endif
> +
>  static pthread_key_t key;
>
>  static inline void lock_mutex(pthread_mutex_t *mutex)
> @@ -124,6 +128,9 @@ static void init_thread(void)
>        init_recursive_mutex(&read_mutex);
>        pthread_mutex_init(&counter_mutex, NULL);
>        pthread_mutex_init(&work_mutex, NULL);
> +#ifdef WIN32
> +       pthread_mutex_init(&pread_mutex, NULL);
> +#endif
>        pthread_key_create(&key, NULL);
>        thread_data = xcalloc(nr_threads, sizeof(*thread_data));
>        threads_active = 1;
> @@ -137,6 +144,9 @@ static void cleanup_thread(void)
>        pthread_mutex_destroy(&read_mutex);
>        pthread_mutex_destroy(&counter_mutex);
>        pthread_mutex_destroy(&work_mutex);
> +#ifdef WIN32
> +       pthread_mutex_destroy(&pread_mutex);
> +#endif
>        pthread_key_delete(key);
>        free(thread_data);
>  }
> @@ -456,6 +466,20 @@ static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_
>        return data;
>  }
>
> +#ifdef WIN32
> +/* pread is reimplemented in compat/pread.c and is not thread-safe */
> +static ssize_t pread_threadsafe(int fd, void *buf, size_t count, off_t from)
> +{
> +       ssize_t n;
> +       lock_mutex(&pread_mutex);
> +       n = pread(fd, buf, count, from);
> +       unlock_mutex(&pread_mutex);
> +       return n;
> +}
> +#else
> +#define pread_threadsafe(fd,buf,count,from) pread(fd, buf, count, from)
> +#endif
> +
>  static void *get_data_from_pack(struct object_entry *obj)
>  {
>        off_t from = obj[0].idx.offset + obj[0].hdr_size;
> @@ -474,7 +498,7 @@ static void *get_data_from_pack(struct object_entry *obj)
>
>        do {
>                ssize_t n = (len < 64*1024) ? len : 64*1024;
> -               n = pread(pack_fd, inbuf, n, from);
> +               n = pread_threadsafe(pack_fd, inbuf, n, from);
>                if (n < 0)
>                        die_errno("cannot pread pack file");
>                if (!n)
> -- 8< --

This approach has the problem that file-operations apart from pread
might (at least in theory) modify the position. To prevent that, we'd
either need to use the same locking-mechanism as the CRT use, or use
ReadFile with an OVERLAPPED struct, which allows us specify the offset
explicitly. The latter seems better to me, and should look something
like this (note: untested):

diff --git a/Makefile b/Makefile
index 2626216..31a5621 100644
--- a/Makefile
+++ b/Makefile
@@ -1153,7 +1153,6 @@ endif
 ifeq ($(uname_S),Windows)
 	GIT_VERSION := $(GIT_VERSION).MSVC
 	pathsep = ;
-	NO_PREAD = YesPlease
 	NEEDS_CRYPTO_WITH_SSL = YesPlease
 	NO_LIBGEN_H = YesPlease
 	NO_SYS_POLL_H = YesPlease
@@ -1250,7 +1249,6 @@ ifeq ($(uname_S),Minix)
 endif
 ifneq (,$(findstring MINGW,$(uname_S)))
 	pathsep = ;
-	NO_PREAD = YesPlease
 	NEEDS_CRYPTO_WITH_SSL = YesPlease
 	NO_LIBGEN_H = YesPlease
 	NO_SYS_POLL_H = YesPlease
diff --git a/compat/mingw.c b/compat/mingw.c
index 309fa1d..63783db 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -617,6 +617,28 @@ static inline void time_t_to_filetime(time_t t,
FILETIME *ft)
 	ft->dwHighDateTime = winTime >> 32;
 }

+ssize_t mingw_pread(int fd, void *buf, size_t size, off_t offset)
+{
+	OVERLAPPED overlapped = { 0 };
+	DWORD ret;
+
+	HANDLE fh = (HANDLE)_get_osfhandle(fd);
+	if (fh == INVALID_HANDLE_VALUE) {
+		errno = EBADF;
+		return -1;
+	}
+
+	overlapped.Offset = (DWORD)offset;
+	overlapped.OffsetHigh = (DWORD)(offset >> 32);
+
+	if (!ReadFile(fh, buf, size, &ret, &overlapped)) {
+		errno = err_win_to_posix(GetLastError());
+		return -1;
+	}
+
+	return ret;
+}
+
 int mingw_utime (const char *file_name, const struct utimbuf *times)
 {
 	FILETIME mft, aft;
diff --git a/compat/mingw.h b/compat/mingw.h
index c7b2cec..da47302 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -285,6 +285,9 @@ int mingw_fstat(int fd, struct stat *buf);
 #define _stati64(x,y) mingw_stat(x,y)
 #endif

+ssize_t mingw_pread(int fd, void *buf, size_t count, off_t offset);
+#define pread mingw_pread
+
 int mingw_utime(const char *file_name, const struct utimbuf *times);
 #define utime mingw_utime