From 9d2f841a5d39fc25ce722a3904f6ebc9aa112222 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Thu, 2 May 2013 03:03:54 -0700 Subject: [PATCH 1/4] Add extra locking around packfile open We were still seeing a few issues in threaded access to packs. This adds extra locks around the opening of the mwindow to avoid a different race. --- src/pack.c | 44 +++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/src/pack.c b/src/pack.c index f8b621ef8..47534f195 100644 --- a/src/pack.c +++ b/src/pack.c @@ -205,13 +205,18 @@ static int pack_index_check(const char *path, struct git_pack_file *p) if (fd < 0) return fd; - if (p_fstat(fd, &st) < 0 || - !S_ISREG(st.st_mode) || + if (p_fstat(fd, &st) < 0) { + p_close(fd); + giterr_set(GITERR_OS, "Unable to stat pack index '%s'", path); + return -1; + } + + if (!S_ISREG(st.st_mode) || !git__is_sizet(st.st_size) || (idx_size = (size_t)st.st_size) < 4 * 256 + 20 + 20) { p_close(fd); - giterr_set(GITERR_OS, "Failed to check pack index."); + giterr_set(GITERR_ODB, "Invalid pack index '%s'", path); return -1; } @@ -402,7 +407,7 @@ int git_packfile_unpack_header( if (base == NULL) return GIT_EBUFS; - ret = packfile_unpack_header1(&used, size_p, type_p, base, left); + ret = packfile_unpack_header1(&used, size_p, type_p, base, left); git_mwindow_close(w_curs); if (ret == GIT_EBUFS) return ret; @@ -799,9 +804,6 @@ void git_packfile_free(struct git_pack_file *p) if (!p) return; - if (git_mutex_lock(&p->lock) < 0) - return; - cache_free(&p->bases); git_mwindow_free_all(&p->mwf); @@ -813,8 +815,6 @@ void git_packfile_free(struct git_pack_file *p) git__free(p->bad_object_sha1); - git_mutex_unlock(&p->lock); - git_mutex_free(&p->lock); git__free(p); } @@ -829,12 +829,19 @@ static int packfile_open(struct git_pack_file *p) if (!p->index_map.data && pack_index_open(p) < 0) return git_odb__error_notfound("failed to open packfile", NULL); + /* if mwf opened by another thread, return now */ + if (git_mutex_lock(&p->lock) < 0) + return packfile_error("failed to get lock for open"); + + if (p->mwf.fd >= 0) { + git_mutex_unlock(&p->lock); + return 0; + } + /* TODO: open with noatime */ p->mwf.fd = git_futils_open_ro(p->pack_name); - if (p->mwf.fd < 0) { - p->mwf.fd = -1; - return -1; - } + if (p->mwf.fd < 0) + goto cleanup; if (p_fstat(p->mwf.fd, &st) < 0 || git_mwindow_file_register(&p->mwf) < 0) @@ -875,13 +882,20 @@ static int packfile_open(struct git_pack_file *p) idx_sha1 = ((unsigned char *)p->index_map.data) + p->index_map.len - 40; - if (git_oid__cmp(&sha1, (git_oid *)idx_sha1) == 0) - return 0; + if (git_oid__cmp(&sha1, (git_oid *)idx_sha1) != 0) + goto cleanup; + + git_mutex_unlock(&p->lock); + return 0; cleanup: giterr_set(GITERR_OS, "Invalid packfile '%s'", p->pack_name); + p_close(p->mwf.fd); p->mwf.fd = -1; + + git_mutex_unlock(&p->lock); + return -1; } From d82d66c96d11131440deb8f79443ee0b44d40eff Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Thu, 2 May 2013 03:05:21 -0700 Subject: [PATCH 2/4] Extra threading tests We need to hammer the packfile open phase harder in the thread tests, in addition to the cache API. --- tests-clar/object/cache.c | 58 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/tests-clar/object/cache.c b/tests-clar/object/cache.c index a3eba8737..e06760e2a 100644 --- a/tests-clar/object/cache.c +++ b/tests-clar/object/cache.c @@ -5,7 +5,7 @@ static git_repository *g_repo; void test_object_cache__initialize(void) { - cl_git_pass(git_repository_open(&g_repo, cl_fixture("testrepo.git"))); + g_repo = NULL; } void test_object_cache__cleanup(void) @@ -56,6 +56,7 @@ void test_object_cache__cache_everything(void) git_libgit2_opts( GIT_OPT_SET_CACHE_OBJECT_LIMIT, (int)GIT_OBJ_BLOB, (size_t)32767); + cl_git_pass(git_repository_open(&g_repo, cl_fixture("testrepo.git"))); cl_git_pass(git_repository_odb(&odb, g_repo)); start = (int)git_cache_size(&g_repo->objects); @@ -105,6 +106,7 @@ void test_object_cache__cache_no_blobs(void) git_libgit2_opts(GIT_OPT_SET_CACHE_OBJECT_LIMIT, (int)GIT_OBJ_BLOB, (size_t)0); + cl_git_pass(git_repository_open(&g_repo, cl_fixture("testrepo.git"))); cl_git_pass(git_repository_odb(&odb, g_repo)); start = (int)git_cache_size(&g_repo->objects); @@ -189,8 +191,8 @@ static void *cache_raw(void *arg) return arg; } -#define REPEAT 50 -#define THREADCOUNT 20 +#define REPEAT 20 +#define THREADCOUNT 50 void test_object_cache__threadmania(void) { @@ -207,6 +209,8 @@ void test_object_cache__threadmania(void) for (try = 0; try < REPEAT; ++try) { + cl_git_pass(git_repository_open(&g_repo, cl_fixture("testrepo.git"))); + for (th = 0; th < THREADCOUNT; ++th) { data = git__malloc(2 * sizeof(int)); @@ -231,5 +235,53 @@ void test_object_cache__threadmania(void) } #endif + git_repository_free(g_repo); + g_repo = NULL; + } +} + +static void *cache_quick(void *arg) +{ + git_oid oid; + git_object *obj; + + cl_git_pass(git_oid_fromstr(&oid, g_data[4].sha)); + cl_git_pass(git_object_lookup(&obj, g_repo, &oid, GIT_OBJ_ANY)); + cl_assert(g_data[4].type == git_object_type(obj)); + git_object_free(obj); + + return arg; +} + +void test_object_cache__fast_thread_rush(void) +{ + int try, th, data[THREADCOUNT*2]; +#ifdef GIT_THREADS + git_thread t[THREADCOUNT*2]; +#endif + + for (try = 0; try < REPEAT; ++try) { + cl_git_pass(git_repository_open(&g_repo, cl_fixture("testrepo.git"))); + + for (th = 0; th < THREADCOUNT*2; ++th) { + data[th] = th; +#ifdef GIT_THREADS + cl_git_pass( + git_thread_create(&t[th], NULL, cache_quick, &data[th])); +#else + cl_assert(cache_quick(&data[th]) == &data[th]); +#endif + } + +#ifdef GIT_THREADS + for (th = 0; th < THREADCOUNT*2; ++th) { + void *rval; + cl_git_pass(git_thread_join(t[th], &rval)); + cl_assert_equal_i(th, *((int *)rval)); + } +#endif + + git_repository_free(g_repo); + g_repo = NULL; } } From 81b7dec4ee21454775f932717273a90a46f78e1f Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Thu, 2 May 2013 03:06:34 -0700 Subject: [PATCH 3/4] Fix some compile warnings and trailing whitespace --- src/index.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/index.c b/src/index.c index 1771f2957..d4aa475a9 100644 --- a/src/index.c +++ b/src/index.c @@ -193,44 +193,46 @@ static int conflict_name_cmp(const void *a, const void *b) { const git_index_name_entry *name_a = a; const git_index_name_entry *name_b = b; - + if (name_a->ancestor && !name_b->ancestor) return 1; - + if (!name_a->ancestor && name_b->ancestor) return -1; - + if (name_a->ancestor) return strcmp(name_a->ancestor, name_b->ancestor); - + if (!name_a->ours || !name_b->ours) return 0; - + return strcmp(name_a->ours, name_b->ours); } /** * TODO: enable this when resolving case insensitive conflicts */ +#if 0 static int conflict_name_icmp(const void *a, const void *b) { const git_index_name_entry *name_a = a; const git_index_name_entry *name_b = b; - + if (name_a->ancestor && !name_b->ancestor) return 1; - + if (!name_a->ancestor && name_b->ancestor) return -1; - + if (name_a->ancestor) return strcasecmp(name_a->ancestor, name_b->ancestor); - + if (!name_a->ours || !name_b->ours) return 0; - + return strcasecmp(name_a->ours, name_b->ours); } +#endif static int reuc_srch(const void *key, const void *array_member) { From 8c535f3f6879c6796d8107d7eb80dd8b2105621b Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Thu, 2 May 2013 03:34:56 -0700 Subject: [PATCH 4/4] Protect sha1_entry_pos call with mutex There is an occasional assertion failure in sha1_entry_pos from pack_entry_find_index when running threaded. Holding the mutex around the code that grabs the index_map data and processes it makes this assertion failure go away. --- src/pack.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/pack.c b/src/pack.c index 47534f195..1ffad29ae 100644 --- a/src/pack.c +++ b/src/pack.c @@ -1050,24 +1050,24 @@ static int pack_entry_find_offset( const git_oid *short_oid, size_t len) { - const uint32_t *level1_ofs = p->index_map.data; - const unsigned char *index = p->index_map.data; + const uint32_t *level1_ofs; + const unsigned char *index; unsigned hi, lo, stride; int pos, found = 0; const unsigned char *current = 0; *offset_out = 0; - if (index == NULL) { - int error; + if (!p->index_map.data && pack_index_open(p) < 0) + return git_odb__error_notfound("failed to open packfile", NULL); - if ((error = pack_index_open(p)) < 0) - return error; - assert(p->index_map.data); + if (git_mutex_lock(&p->lock) < 0) + return packfile_error("failed to get lock for finding entry offset"); - index = p->index_map.data; - level1_ofs = p->index_map.data; - } + assert(p->index_map.data); + + index = p->index_map.data; + level1_ofs = p->index_map.data; if (p->index_version > 1) { level1_ofs += 2; @@ -1093,6 +1093,8 @@ static int pack_entry_find_offset( /* Use git.git lookup code */ pos = sha1_entry_pos(index, stride, 0, lo, hi, p->num_objects, short_oid->id); + git_mutex_unlock(&p->lock); + if (pos >= 0) { /* An object matching exactly the oid was found */ found = 1;