# [PATCH v4 1/2] pack-objects: move read mutex to packing_data struct 2 messages from 2019-01-25 to 2019-01-25. Participants: Patrick Hogg. Thread: https://gitlist.dev/t/50314 ## Patrick Hogg, 2019-01-25 00:22 Subject: [PATCH v4 1/2] pack-objects: move read mutex to packing_data struct Message-ID: <20190125002203.38756-1-phogg@novamoon.net> URL: https://gitlist.dev/e/20190125002203.38756-1-phogg%40novamoon.net ``` ac77d0c37 ("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 moving the read mutex to packing_data and initializing it in prepare_packing_data which is initialized in cmd_pack_objects. Signed-off-by: Patrick Hogg Reviewed-by: Duy Nguyen --- builtin/pack-objects.c | 7 ++----- pack-objects.c | 1 + pack-objects.h | 10 ++++++++++ 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 411aefd68..506061b4c 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 read_lock() packing_data_read_lock(&to_pack) +#define read_unlock() packing_data_read_unlock(&to_pack) /* Protect delta_cache_size */ static pthread_mutex_t cache_mutex; @@ -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..3554c43ac 100644 --- a/pack-objects.c +++ b/pack-objects.c @@ -150,6 +150,7 @@ void prepare_packing_data(struct packing_data *pdata) 1UL << OE_DELTA_SIZE_BITS); #ifndef NO_PTHREADS pthread_mutex_init(&pdata->lock, NULL); + init_recursive_mutex(&pdata->read_lock); #endif } diff --git a/pack-objects.h b/pack-objects.h index dc869f26c..0a038e3bc 100644 --- a/pack-objects.h +++ b/pack-objects.h @@ -146,6 +146,7 @@ struct packing_data { struct packed_git **in_pack; pthread_mutex_t lock; + pthread_mutex_t read_lock; /* * This list contains entries for bases which we know the other side @@ -174,6 +175,15 @@ static inline void packing_data_unlock(struct packing_data *pdata) pthread_mutex_unlock(&pdata->lock); } +static inline void packing_data_read_lock(struct packing_data *pdata) +{ + pthread_mutex_lock(&pdata->read_lock); +} +static inline void packing_data_read_unlock(struct packing_data *pdata) +{ + pthread_mutex_unlock(&pdata->read_lock); +} + struct object_entry *packlist_alloc(struct packing_data *pdata, const unsigned char *sha1, uint32_t index_pos); -- 2.20.1.windows.1 ``` ## Patrick Hogg, 2019-01-25 00:22 Subject: [PATCH v4 2/2] pack-objects: merge read_lock and lock in packing_data struct Message-ID: <20190125002203.38756-2-phogg@novamoon.net> URL: https://gitlist.dev/e/20190125002203.38756-2-phogg%40novamoon.net In-Reply-To: <20190125002203.38756-1-phogg@novamoon.net> ``` Rename the packing_data lock to obd_lock and upgrade it to a recursive mutex to make it suitable for current read_lock usages. Additionally remove the superfluous #ifndef NO_PTHREADS guard around mutex initialization in prepare_packing_data as the mutex functions themselves are already protected. Signed-off-by: Patrick Hogg Thanks-to: Junio C Hamano --- builtin/pack-objects.c | 24 ++++++++++-------------- pack-objects.c | 5 +---- pack-objects.h | 21 ++++++++------------- 3 files changed, 19 insertions(+), 31 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 506061b4c..cfef48217 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1953,10 +1953,6 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size, return 0; } -/* Protect access to object database */ -#define read_lock() packing_data_read_lock(&to_pack) -#define read_unlock() packing_data_read_unlock(&to_pack) - /* Protect delta_cache_size */ static pthread_mutex_t cache_mutex; #define cache_lock() pthread_mutex_lock(&cache_mutex) @@ -1992,11 +1988,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(); + packing_data_lock(&to_pack); 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(); + packing_data_unlock(&to_pack); return size; } @@ -2004,7 +2000,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(); + packing_data_lock(&to_pack); w_curs = NULL; buf = use_pack(p, &w_curs, e->in_pack_offset, &avail); used = unpack_object_header_buffer(buf, avail, &type, &size); @@ -2013,7 +2009,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack, oid_to_hex(&e->idx.oid)); unuse_pack(&w_curs); - read_unlock(); + packing_data_unlock(&to_pack); return size; } @@ -2075,9 +2071,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src, /* Load data if not already done */ if (!trg->data) { - read_lock(); + packing_data_lock(&to_pack); trg->data = read_object_file(&trg_entry->idx.oid, &type, &sz); - read_unlock(); + packing_data_unlock(&to_pack); if (!trg->data) die(_("object %s cannot be read"), oid_to_hex(&trg_entry->idx.oid)); @@ -2088,9 +2084,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src, *mem_usage += sz; } if (!src->data) { - read_lock(); + packing_data_lock(&to_pack); src->data = read_object_file(&src_entry->idx.oid, &type, &sz); - read_unlock(); + packing_data_unlock(&to_pack); if (!src->data) { if (src_entry->preferred_base) { static int warned = 0; @@ -2336,9 +2332,9 @@ static void find_deltas(struct object_entry **list, unsigned *list_size, static void try_to_free_from_threads(size_t size) { - read_lock(); + packing_data_lock(&to_pack); release_pack_memory(size); - read_unlock(); + packing_data_unlock(&to_pack); } static try_to_free_t old_try_to_free_routine; diff --git a/pack-objects.c b/pack-objects.c index 3554c43ac..a1dc5eb72 100644 --- a/pack-objects.c +++ b/pack-objects.c @@ -148,10 +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); - init_recursive_mutex(&pdata->read_lock); -#endif + init_recursive_mutex(&pdata->odb_lock); } struct object_entry *packlist_alloc(struct packing_data *pdata, diff --git a/pack-objects.h b/pack-objects.h index 0a038e3bc..1667cbad8 100644 --- a/pack-objects.h +++ b/pack-objects.h @@ -145,8 +145,11 @@ struct packing_data { struct packed_git **in_pack_by_idx; struct packed_git **in_pack; - pthread_mutex_t lock; - pthread_mutex_t read_lock; + /* + * During packing with multiple threads, protect the in-core + * object database from concurrent accesses. + */ + pthread_mutex_t odb_lock; /* * This list contains entries for bases which we know the other side @@ -166,22 +169,14 @@ struct packing_data { void prepare_packing_data(struct packing_data *pdata); +/* Protect access to object database */ static inline void packing_data_lock(struct packing_data *pdata) { - pthread_mutex_lock(&pdata->lock); + pthread_mutex_lock(&pdata->odb_lock); } static inline void packing_data_unlock(struct packing_data *pdata) { - pthread_mutex_unlock(&pdata->lock); -} - -static inline void packing_data_read_lock(struct packing_data *pdata) -{ - pthread_mutex_lock(&pdata->read_lock); -} -static inline void packing_data_read_unlock(struct packing_data *pdata) -{ - pthread_mutex_unlock(&pdata->read_lock); + pthread_mutex_unlock(&pdata->odb_lock); } struct object_entry *packlist_alloc(struct packing_data *pdata, -- 2.20.1.windows.1 ```