Re: What's cooking in git.git (Apr 2012, #06; Sun, 15)
- From
Erik Faye-Lund <kusmabite@gmail.com>
- Date
- Apr 19, 2012, 12:58 UTC
- Message-ID
- <CABPQNSZ6VdyoLcVUUJ4z5A2A7KGP8qBZAkzdx8zAtAs2mZN25w@mail.gmail.com>
- In-Reply-To
- <20120419093608.GA19391@duynguyen-vnpc.dek-tpc.internal>
On Thu, Apr 19, 2012 at 11:36 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
Show 76 quoted lines
> 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