Merge pull request #1529 from arrbee/more-packfile-locking

More packfile locking

3 files changed, 108 insertions, 38 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)
 {
diff --git a/src/pack.c b/src/pack.c
index f8b621ef8..1ffad29ae 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;
 }
 
@@ -1036,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;
@@ -1079,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;
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;
 	}
 }