Add mutex around mapping and unmapping pack files When I was writing threading tests for the new cache, the main error I kept running into was a pack file having it's content unmapped underneath the running thread. This adds a lock around the routines that map and unmap the pack data so that threads can effectively reload the data when they need it. This also required reworking the error handling paths in a couple places in the code which I tried to make consistent.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173
diff --git a/src/pack.c b/src/pack.c
index 75ac981..1bffb47 100644
--- a/src/pack.c
+++ b/src/pack.c
@@ -296,24 +296,34 @@ static int pack_index_check(const char *path, struct git_pack_file *p)
static int pack_index_open(struct git_pack_file *p)
{
char *idx_name;
- int error;
- size_t name_len, offset;
+ int error = 0;
+ size_t name_len, base_len;
+
+ if ((error = git_mutex_lock(&p->lock)) < 0)
+ return error;
if (p->index_map.data)
- return 0;
+ goto done;
- idx_name = git__strdup(p->pack_name);
- GITERR_CHECK_ALLOC(idx_name);
+ name_len = strlen(p->pack_name);
+ assert(name_len > strlen(".pack")); /* checked by git_pack_file alloc */
- name_len = strlen(idx_name);
- offset = name_len - strlen(".pack");
- assert(offset < name_len); /* make sure no underflow */
+ if ((idx_name = git__malloc(name_len)) == NULL) {
+ error = -1;
+ goto done;
+ }
- strncpy(idx_name + offset, ".idx", name_len - offset);
+ base_len = name_len - strlen(".pack");
+ memcpy(idx_name, p->pack_name, base_len);
+ memcpy(idx_name + base_len, ".idx", sizeof(".idx"));
error = pack_index_check(idx_name, p);
+
git__free(idx_name);
+done:
+ git_mutex_unlock(&p->lock);
+
return error;
}
@@ -389,7 +399,7 @@ int git_packfile_unpack_header(
* the maximum deflated object size is 2^137, which is just
* insane, so we know won't exceed what we have been given.
*/
-// base = pack_window_open(p, w_curs, *curpos, &left);
+/* base = pack_window_open(p, w_curs, *curpos, &left); */
base = git_mwindow_open(mwf, w_curs, *curpos, 20, &left);
if (base == NULL)
return GIT_EBUFS;
@@ -789,15 +799,23 @@ git_off_t get_delta_base(
static struct git_pack_file *packfile_alloc(size_t extra)
{
struct git_pack_file *p = git__calloc(1, sizeof(*p) + extra);
- if (p != NULL)
- p->mwf.fd = -1;
+ if (!p)
+ return NULL;
+
+ p->mwf.fd = -1;
+ git_mutex_init(&p->lock);
+
return p;
}
void git_packfile_free(struct git_pack_file *p)
{
- assert(p);
+ if (!p)
+ return;
+
+ if (git_mutex_lock(&p->lock) < 0)
+ return;
cache_free(&p->bases);
@@ -810,6 +828,10 @@ void git_packfile_free(struct git_pack_file *p)
pack_index_free(p);
git__free(p->bad_object_sha1);
+
+ git_mutex_unlock(&p->lock);
+
+ git_mutex_free(&p->lock);
git__free(p);
}
@@ -820,8 +842,6 @@ static int packfile_open(struct git_pack_file *p)
git_oid sha1;
unsigned char *idx_sha1;
- assert(p->index_map.data);
-
if (!p->index_map.data && pack_index_open(p) < 0)
return git_odb__error_notfound("failed to open packfile", NULL);
@@ -888,7 +908,10 @@ int git_packfile_check(struct git_pack_file **pack_out, const char *path)
size_t path_len;
*pack_out = NULL;
- path_len = strlen(path);
+
+ if (!path || (path_len = strlen(path)) <= strlen(".idx"))
+ return git_odb__error_notfound("invalid packfile path", NULL);
+
p = packfile_alloc(path_len + 2);
GITERR_CHECK_ALLOC(p);
@@ -897,18 +920,13 @@ int git_packfile_check(struct git_pack_file **pack_out, const char *path)
* the index looks sane.
*/
path_len -= strlen(".idx");
- if (path_len < 1) {
- git__free(p);
- return git_odb__error_notfound("invalid packfile path", NULL);
- }
-
memcpy(p->pack_name, path, path_len);
- strcpy(p->pack_name + path_len, ".keep");
+ memcpy(p->pack_name + path_len, ".keep", sizeof(".keep"));
if (git_path_exists(p->pack_name) == true)
p->pack_keep = 1;
- strcpy(p->pack_name + path_len, ".pack");
+ memcpy(p->pack_name + path_len, ".pack", sizeof(".pack"));
if (p_stat(p->pack_name, &st) < 0 || !S_ISREG(st.st_mode)) {
git__free(p);
return git_odb__error_notfound("packfile not found", NULL);
@@ -1039,7 +1057,6 @@ static int pack_entry_find_offset(
if ((error = pack_index_open(p)) < 0)
return error;
-
assert(p->index_map.data);
index = p->index_map.data;
@@ -1099,6 +1116,7 @@ static int pack_entry_find_offset(
return git_odb__error_notfound("failed to find offset for pack entry", short_oid);
if (found > 1)
return git_odb__error_ambiguous("found multiple offsets for pack entry");
+
*offset_out = nth_packed_object_offset(p, pos);
git_oid_fromraw(found_oid, current);
@@ -1110,6 +1128,7 @@ static int pack_entry_find_offset(
printf("found lo=%d %s\n", lo, hex_sha1);
}
#endif
+
return 0;
}
diff --git a/src/pack.h b/src/pack.h
index 8d7e33d..b734ac1 100644
--- a/src/pack.h
+++ b/src/pack.h
@@ -79,6 +79,7 @@ typedef struct {
struct git_pack_file {
git_mwindow_file mwf;
git_map index_map;
+ git_mutex lock;
uint32_t num_objects;
uint32_t num_bad_objects;