index-pack: work around thread-unsafe pread()

Multi-threaing of index-pack was disabled with c0f8654
(index-pack: Disable threading on cygwin - 2012-06-26), because
pread() implementations for Cygwin and MSYS were not thread
safe.  Recent Cygwin does offer usable pread() and we enabled
multi-threading with 103d530f (Cygwin 1.7 has thread-safe pread,
2013-07-19).

Work around this problem on platforms with a thread-unsafe
pread() emulation by opening one file handle per thread; it
would prevent parallel pread() on different file handles from
stepping on each other.

Also remove NO_THREAD_SAFE_PREAD that was introduced in c0f8654
because it's no longer used anywhere.

This workaround is unconditional, even for platforms with
thread-safe pread() because the overhead is small (a couple file
handles more) and not worth fragmenting the code.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Tested-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Makefile b/Makefile
index dddaf4f287cf5cd5e99ad2587d53ba7582c51e29..00a70840eb17d70601e55ad654e4ca75e2b26883 100644 (file)
--- a/Makefile
+++ b/Makefile
@@ -191,9 +191,6 @@ all::
 # Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval
 # This also implies NO_SETITIMER
 #
-# Define NO_THREAD_SAFE_PREAD if your pread() implementation is not
-# thread-safe. (e.g. compat/pread.c or cygwin)
-#
 # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is
 # generally faster on your platform than accessing the working directory.
 #
@@ -1341,10 +1338,6 @@ endif
 ifdef NO_PREAD
        COMPAT_CFLAGS += -DNO_PREAD
        COMPAT_OBJS += compat/pread.o
-       NO_THREAD_SAFE_PREAD = YesPlease
-endif
-ifdef NO_THREAD_SAFE_PREAD
-       BASIC_CFLAGS += -DNO_THREAD_SAFE_PREAD
 endif
 ifdef NO_FAST_WORKING_DIRECTORY
        BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2f37a38fbcdf131ab5897e47728c5773ba36b861..ed489bbe9aab3ae7268f139f2d34c737880be49c 100644 (file)
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -40,17 +40,13 @@ struct base_data {
        int ofs_first, ofs_last;
 };
 
-#if !defined(NO_PTHREADS) && defined(NO_THREAD_SAFE_PREAD)
-/* pread() emulation is not thread-safe. Disable threading. */
-#define NO_PTHREADS
-#endif
-
 struct thread_local {
 #ifndef NO_PTHREADS
        pthread_t thread;
 #endif
        struct base_data *base_cache;
        size_t base_cache_used;
+       int pack_fd;
 };
 
 /*
@@ -91,7 +87,8 @@ static off_t consumed_bytes;
 static unsigned deepest_delta;
 static git_SHA_CTX input_ctx;
 static uint32_t input_crc32;
-static int input_fd, output_fd, pack_fd;
+static int input_fd, output_fd;
+static const char *curr_pack;
 
 #ifndef NO_PTHREADS
 
@@ -134,6 +131,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)
  */
 static void init_thread(void)
 {
+       int i;
        init_recursive_mutex(&read_mutex);
        pthread_mutex_init(&counter_mutex, NULL);
        pthread_mutex_init(&work_mutex, NULL);
@@ -141,11 +139,18 @@ static void init_thread(void)
                pthread_mutex_init(&deepest_delta_mutex, NULL);
        pthread_key_create(&key, NULL);
        thread_data = xcalloc(nr_threads, sizeof(*thread_data));
+       for (i = 0; i < nr_threads; i++) {
+               thread_data[i].pack_fd = open(curr_pack, O_RDONLY);
+               if (thread_data[i].pack_fd == -1)
+                       die_errno(_("unable to open %s"), curr_pack);
+       }
+
        threads_active = 1;
 }
 
 static void cleanup_thread(void)
 {
+       int i;
        if (!threads_active)
                return;
        threads_active = 0;
@@ -154,6 +159,8 @@ static void cleanup_thread(void)
        pthread_mutex_destroy(&work_mutex);
        if (show_stat)
                pthread_mutex_destroy(&deepest_delta_mutex);
+       for (i = 0; i < nr_threads; i++)
+               close(thread_data[i].pack_fd);
        pthread_key_delete(key);
        free(thread_data);
 }
@@ -288,13 +295,13 @@ static const char *open_pack_file(const char *pack_name)
                        output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);
                if (output_fd < 0)
                        die_errno(_("unable to create '%s'"), pack_name);
-               pack_fd = output_fd;
+               nothread_data.pack_fd = output_fd;
        } else {
                input_fd = open(pack_name, O_RDONLY);
                if (input_fd < 0)
                        die_errno(_("cannot open packfile '%s'"), pack_name);
                output_fd = -1;
-               pack_fd = input_fd;
+               nothread_data.pack_fd = input_fd;
        }
        git_SHA1_Init(&input_ctx);
        return pack_name;
@@ -542,7 +549,7 @@ static void *unpack_data(struct object_entry *obj,
 
        do {
                ssize_t n = (len < 64*1024) ? len : 64*1024;
-               n = pread(pack_fd, inbuf, n, from);
+               n = pread(get_thread_data()->pack_fd, inbuf, n, from);
                if (n < 0)
                        die_errno(_("cannot pread pack file"));
                if (!n)
@@ -1490,7 +1497,7 @@ static void show_pack_info(int stat_only)
 int cmd_index_pack(int argc, const char **argv, const char *prefix)
 {
        int i, fix_thin_pack = 0, verify = 0, stat_only = 0;
-       const char *curr_pack, *curr_index;
+       const char *curr_index;
        const char *index_name = NULL, *pack_name = NULL;
        const char *keep_name = NULL, *keep_msg = NULL;
        char *index_name_buf = NULL, *keep_name_buf = NULL;

diff --git a/config.mak.uname b/config.mak.uname
index 7d31fad241761e41de655de6f42ddfb8c87ea4f2..db5c249b4ca291f9b3ba27b9619430fccbf7d1ff 100644 (file)
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -158,7 +158,6 @@ ifeq ($(uname_O),Cygwin)
                NO_SYMLINK_HEAD = YesPlease
                NO_IPV6 = YesPlease
                OLD_ICONV = UnfortunatelyYes
-               NO_THREAD_SAFE_PREAD = YesPlease
                # There are conflicting reports about this.
                # On some boxes NO_MMAP is needed, and not so elsewhere.
                # Try commenting this out if you suspect MMAP is more efficient