Patrick Hoggac77d0c37 ("pack-objects: shrink size field in struct object_entry", 2018-04-14) added an extra usage of read_lock/read_unlock in the newly introduced oe_get_size_slow for thread safety in parallel calls to try_delta(). Unfortunately oe_get_size_slow is also used in serial code, some of which is called before the first invocation of ll_find_deltas. As such the read mutex is not guaranteed to be initialized.
Resolve this by using the existing lock in packing_data which is initialized early in cmd_pack_objects instead of read_mutex. Additionally, upgrade the packing_data lock to a recursive mutex to make it a suitable replacement for read_mutex.
Signed-off-by: Patrick Hogg <phogg@novamoon.net> ---
As I mentioned in the prior thread I think that it will be simpler to simply use the existing lock in packing_data instead of moving read_mutex. I can go back to simply moving read_mutex to the packing_data struct if that that is preferable, though.
I also removed the #ifndef NO_PTHREADS in prepare_packing_data around the initialization of &pdata->lock since I had to upgrade the lock to a recursive mutex. As far as I can tell init_recursive_mutex (and pthread_mutex_init for that matter) have that protection already so it appears to be redundant.
builtin/pack-objects.c | 27 ++++++++++++--------------- pack-objects.c | 4 +--- 2 files changed, 13 insertions(+), 18 deletions(-)
Show changes to 2 files +13 −18
builtin/pack-objects.c, pack-objects.c
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 411aefd68..5439b434c 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1954,9 +1954,8 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size, } /* Protect access to object database */ -static pthread_mutex_t read_mutex; -#define read_lock() pthread_mutex_lock(&read_mutex) -#define read_unlock() pthread_mutex_unlock(&read_mutex) +#define pack_lock() packing_data_lock(&to_pack) +#define pack_unlock() packing_data_unlock(&to_pack) /* Protect delta_cache_size */ static pthread_mutex_t cache_mutex; @@ -1993,11 +1992,11 @@ unsigned long oe_get_size_slow(struct packing_data *pack, unsigned long used, avail, size; if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) { - read_lock(); + pack_lock(); if (oid_object_info(the_repository, &e->idx.oid, &size) < 0) die(_("unable to get size of %s"), oid_to_hex(&e->idx.oid)); - read_unlock(); + pack_unlock(); return size; } @@ -2005,7 +2004,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack, if (!p) BUG("when e->type is a delta, it must belong to a pack"); - read_lock(); + pack_lock(); w_curs = NULL; buf = use_pack(p, &w_curs, e->in_pack_offset, &avail); used = unpack_object_header_buffer(buf, avail, &type, &size); @@ -2014,7 +2013,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack, oid_to_hex(&e->idx.oid)); unuse_pack(&w_curs); - read_unlock(); + pack_unlock(); return size; } @@ -2076,9 +2075,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src, /* Load data if not already done */ if (!trg->data) { - read_lock(); + pack_lock(); trg->data = read_object_file(&trg_entry->idx.oid, &type, &sz); - read_unlock(); + pack_unlock(); if (!trg->data) die(_("object %s cannot be read"), oid_to_hex(&trg_entry->idx.oid)); @@ -2089,9 +2088,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src, *mem_usage += sz; } if (!src->data) { - read_lock(); + pack_lock(); src->data = read_object_file(&src_entry->idx.oid, &type, &sz); - read_unlock(); + pack_unlock(); if (!src->data) { if (src_entry->preferred_base) { static int warned = 0; @@ -2337,9 +2336,9 @@ static void find_deltas(struct object_entry **list, unsigned *list_size, static void try_to_free_from_threads(size_t size) { - read_lock(); + pack_lock(); release_pack_memory(size); - read_unlock(); + pack_unlock(); } static try_to_free_t old_try_to_free_routine; @@ -2381,7 +2380,6 @@ static pthread_cond_t progress_cond; */ static void init_threaded_search(void) { - init_recursive_mutex(&read_mutex); pthread_mutex_init(&cache_mutex, NULL); pthread_mutex_init(&progress_mutex, NULL); pthread_cond_init(&progress_cond, NULL); @@ -2392,7 +2390,6 @@ static void cleanup_threaded_search(void) { set_try_to_free_routine(old_try_to_free_routine); pthread_cond_destroy(&progress_cond); - pthread_mutex_destroy(&read_mutex); pthread_mutex_destroy(&cache_mutex); pthread_mutex_destroy(&progress_mutex); } diff --git a/pack-objects.c b/pack-objects.c index b6cdbb016..6f32a7ba0 100644 --- a/pack-objects.c +++ b/pack-objects.c @@ -148,9 +148,7 @@ void prepare_packing_data(struct packing_data *pdata) 1U << OE_SIZE_BITS); pdata->oe_delta_size_limit = git_env_ulong("GIT_TEST_OE_DELTA_SIZE", 1UL << OE_DELTA_SIZE_BITS); -#ifndef NO_PTHREADS - pthread_mutex_init(&pdata->lock, NULL); -#endif + init_recursive_mutex(&pdata->lock); } struct object_entry *packlist_alloc(struct packing_data *pdata,
-- 2.20.1.windows.1
