@matheusbsbs / matheus-bagliano / commits / ab791dd138

index-pack: fix race condition with duplicate bases

When we are resolving deltas in an indexed pack, we do it by first selecting a potential base (either one stored in full in the pack, or one created by resolving another delta), and then resolving any deltas that use that base. When we resolve a particular delta, we flip its "real_type" field from OBJ_{REF,OFS}_DELTA to whatever the real type is. We assume that traversing the objects this way will visit each delta only once. This is correct for most packs; we visit the delta only when we process its base, and each object (and thus each base) appears only once. However, if a base object appears multiple times in the pack, we will try to resolve any deltas based on it once for each instance. We can detect this case by noting that a delta we are about to resolve has already had its real_type field flipped, and we already do so with an assert(). However, if multiple threads are in use, we may race with another thread on comparing and flipping the field. We need to synchronize the access. The right mechanism for doing this is a compare-and-swap (we atomically "claim" the delta for our own and find out whether our claim was successful). We can implement this in C by using a pthread mutex to protect the operation. This is not the fastest way of doing a compare-and-swap; many processors provide instructions for this, and gcc and other compilers provide builtins to access them. However, some experiments showed that lock contention does not cause a significant slowdown here. Adding c-a-s support for many compilers would increase the maintenance burden (and we would still end up including the pthread version as a fallback). Note that we only need to touch the OBJ_REF_DELTA codepath here. An OBJ_OFS_DELTA object points to its base using an offset, and therefore has only one base, even if another copy of that base object appears in the pack (we do still touch it briefly because the setting of real_type is factored out of resolve_data). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Aug 29, 2014 at 16:57 UTC ab791dd1383536405b8c68ffcf84246b3b0a9812

1 file changed +31 -2

builtin/index-pack.c

+31 -2

@@ -115,6 +115,10 @@ static pthread_mutex_t deepest_delta_mutex;

115 #define deepest_delta_lock() lock_mutex(&deepest_delta_mutex)

116 #define deepest_delta_unlock() unlock_mutex(&deepest_delta_mutex)

117

118 +static pthread_mutex_t type_cas_mutex;

119 +#define type_cas_lock() lock_mutex(&type_cas_mutex)

120 +#define type_cas_unlock() unlock_mutex(&type_cas_mutex)

121 +

122 static pthread_key_t key;

123

124 static inline void lock_mutex(pthread_mutex_t *mutex)

@@ -137,6 +141,7 @@ static void init_thread(void)

141 init_recursive_mutex(&read_mutex);

142 pthread_mutex_init(&counter_mutex, NULL);

143 pthread_mutex_init(&work_mutex, NULL);

144 + pthread_mutex_init(&type_cas_mutex, NULL);

145 if (show_stat)

146 pthread_mutex_init(&deepest_delta_mutex, NULL);

147 pthread_key_create(&key, NULL);

@@ -152,6 +157,7 @@ static void cleanup_thread(void)

157 pthread_mutex_destroy(&read_mutex);

158 pthread_mutex_destroy(&counter_mutex);

159 pthread_mutex_destroy(&work_mutex);

160 + pthread_mutex_destroy(&type_cas_mutex);

161 if (show_stat)

162 pthread_mutex_destroy(&deepest_delta_mutex);

163 pthread_key_delete(key);

@@ -850,7 +856,6 @@ static void resolve_delta(struct object_entry *delta_obj,

856 {

857 void *base_data, *delta_data;

858

853 - delta_obj->real_type = base->obj->real_type;

859 if (show_stat) {

860 delta_obj->delta_depth = base->obj->delta_depth + 1;

861 deepest_delta_lock();

@@ -876,6 +881,26 @@ static void resolve_delta(struct object_entry *delta_obj,

881 counter_unlock();

882 }

883

884 +/*

885 + * Standard boolean compare-and-swap: atomically check whether "*type" is

886 + * "want"; if so, swap in "set" and return true. Otherwise, leave it untouched

887 + * and return false.

888 + */

889 +static int compare_and_swap_type(enum object_type *type,

890 + enum object_type want,

891 + enum object_type set)

892 +{

893 + enum object_type old;

894 +

895 + type_cas_lock();

896 + old = *type;

897 + if (old == want)

898 + *type = set;

899 + type_cas_unlock();

900 +

901 + return old == want;

902 +}

903 +

904 static struct base_data *find_unresolved_deltas_1(struct base_data *base,

905 struct base_data *prev_base)

906 {

@@ -903,7 +928,10 @@ static struct base_data *find_unresolved_deltas_1(struct base_data *base,

928 struct object_entry *child = objects + deltas[base->ref_first].obj_no;

929 struct base_data *result = alloc_base_data();

930

906 - assert(child->real_type == OBJ_REF_DELTA);

931 + if (!compare_and_swap_type(&child->real_type, OBJ_REF_DELTA,

932 + base->obj->real_type))

933 + die("BUG: child->real_type != OBJ_REF_DELTA");

934 +

935 resolve_delta(child, base, result);

936 if (base->ref_first == base->ref_last && base->ofs_last == -1)

937 free_base_data(base);

@@ -917,6 +945,7 @@ static struct base_data *find_unresolved_deltas_1(struct base_data *base,

945 struct base_data *result = alloc_base_data();

946

947 assert(child->real_type == OBJ_OFS_DELTA);

948 + child->real_type = base->obj->real_type;

949 resolve_delta(child, base, result);

950 if (base->ofs_first == base->ofs_last)

951 free_base_data(base);