{"thread":{"id":"50314","subject":"[PATCH v4 1/2] pack-objects: move read mutex to packing_data struct","startedAt":"2019-01-25T00:22:51Z","lastAt":"2019-01-25T00:23:23Z","messageCount":2,"participants":["Patrick Hogg"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"367630","messageId":"20190125002203.38756-1-phogg@novamoon.net","threadId":"50314","inReplyTo":null,"subject":"[PATCH v4 1/2] pack-objects: move read mutex to packing_data struct","fromName":"Patrick Hogg","fromEmail":"phogg@novamoon.net","sentAt":"2019-01-25T00:22:03Z","receivedAt":"2019-01-25T00:22:51Z","isPatch":true,"sender":{"key":"phogg@novamoon.net","avatar":"https://avatars.githubusercontent.com/u/7890416?v=4"},"body":"ac77d0c37 (\"pack-objects: shrink size field in struct object_entry\",\n2018-04-14) added an extra usage of read_lock/read_unlock in the newly\nintroduced oe_get_size_slow for thread safety in parallel calls to\ntry_delta(). Unfortunately oe_get_size_slow is also used in serial\ncode, some of which is called before the first invocation of\nll_find_deltas. As such the read mutex is not guaranteed to be\ninitialized.\n\nResolve this by moving the read mutex to packing_data and initializing\nit in prepare_packing_data which is initialized in cmd_pack_objects.\n\nSigned-off-by: Patrick Hogg <phogg@novamoon.net>\nReviewed-by: Duy Nguyen <pclouds@gmail.com>\n---\n builtin/pack-objects.c |  7 ++-----\n pack-objects.c         |  1 +\n pack-objects.h         | 10 ++++++++++\n 3 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 411aefd68..506061b4c 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1954,9 +1954,8 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,\n }\n \n /* Protect access to object database */\n-static pthread_mutex_t read_mutex;\n-#define read_lock()\t\tpthread_mutex_lock(&read_mutex)\n-#define read_unlock()\t\tpthread_mutex_unlock(&read_mutex)\n+#define read_lock()\t\tpacking_data_read_lock(&to_pack)\n+#define read_unlock()\t\tpacking_data_read_unlock(&to_pack)\n \n /* Protect delta_cache_size */\n static pthread_mutex_t cache_mutex;\n@@ -2381,7 +2380,6 @@ static pthread_cond_t progress_cond;\n  */\n static void init_threaded_search(void)\n {\n-\tinit_recursive_mutex(&read_mutex);\n \tpthread_mutex_init(&cache_mutex, NULL);\n \tpthread_mutex_init(&progress_mutex, NULL);\n \tpthread_cond_init(&progress_cond, NULL);\n@@ -2392,7 +2390,6 @@ static void cleanup_threaded_search(void)\n {\n \tset_try_to_free_routine(old_try_to_free_routine);\n \tpthread_cond_destroy(&progress_cond);\n-\tpthread_mutex_destroy(&read_mutex);\n \tpthread_mutex_destroy(&cache_mutex);\n \tpthread_mutex_destroy(&progress_mutex);\n }\ndiff --git a/pack-objects.c b/pack-objects.c\nindex b6cdbb016..3554c43ac 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -150,6 +150,7 @@ void prepare_packing_data(struct packing_data *pdata)\n \t\t\t\t\t\t   1UL << OE_DELTA_SIZE_BITS);\n #ifndef NO_PTHREADS\n \tpthread_mutex_init(&pdata->lock, NULL);\n+\tinit_recursive_mutex(&pdata->read_lock);\n #endif\n }\n \ndiff --git a/pack-objects.h b/pack-objects.h\nindex dc869f26c..0a038e3bc 100644\n--- a/pack-objects.h\n+++ b/pack-objects.h\n@@ -146,6 +146,7 @@ struct packing_data {\n \tstruct packed_git **in_pack;\n \n \tpthread_mutex_t lock;\n+\tpthread_mutex_t read_lock;\n \n \t/*\n \t * This list contains entries for bases which we know the other side\n@@ -174,6 +175,15 @@ static inline void packing_data_unlock(struct packing_data *pdata)\n \tpthread_mutex_unlock(&pdata->lock);\n }\n \n+static inline void packing_data_read_lock(struct packing_data *pdata)\n+{\n+\tpthread_mutex_lock(&pdata->read_lock);\n+}\n+static inline void packing_data_read_unlock(struct packing_data *pdata)\n+{\n+\tpthread_mutex_unlock(&pdata->read_lock);\n+}\n+\n struct object_entry *packlist_alloc(struct packing_data *pdata,\n \t\t\t\t    const unsigned char *sha1,\n \t\t\t\t    uint32_t index_pos);\n-- \n2.20.1.windows.1\n\n"},{"id":"367632","messageId":"20190125002203.38756-2-phogg@novamoon.net","threadId":"50314","inReplyTo":"20190125002203.38756-1-phogg@novamoon.net","subject":"[PATCH v4 2/2] pack-objects: merge read_lock and lock in packing_data struct","fromName":"Patrick Hogg","fromEmail":"phogg@novamoon.net","sentAt":"2019-01-25T00:22:05Z","receivedAt":"2019-01-25T00:23:23Z","isPatch":true,"sender":{"key":"phogg@novamoon.net","avatar":"https://avatars.githubusercontent.com/u/7890416?v=4"},"body":"Rename the packing_data lock to obd_lock and upgrade it to a recursive\nmutex to make it suitable for current read_lock usages. Additionally\nremove the superfluous #ifndef NO_PTHREADS guard around mutex\ninitialization in prepare_packing_data as the mutex functions\nthemselves are already protected.\n\nSigned-off-by: Patrick Hogg <phogg@novamoon.net>\nThanks-to: Junio C Hamano <gitster@pobox.com>\n---\n builtin/pack-objects.c | 24 ++++++++++--------------\n pack-objects.c         |  5 +----\n pack-objects.h         | 21 ++++++++-------------\n 3 files changed, 19 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 506061b4c..cfef48217 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1953,10 +1953,6 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,\n \treturn 0;\n }\n \n-/* Protect access to object database */\n-#define read_lock()\t\tpacking_data_read_lock(&to_pack)\n-#define read_unlock()\t\tpacking_data_read_unlock(&to_pack)\n-\n /* Protect delta_cache_size */\n static pthread_mutex_t cache_mutex;\n #define cache_lock()\t\tpthread_mutex_lock(&cache_mutex)\n@@ -1992,11 +1988,11 @@ unsigned long oe_get_size_slow(struct packing_data *pack,\n \tunsigned long used, avail, size;\n \n \tif (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {\n-\t\tread_lock();\n+\t\tpacking_data_lock(&to_pack);\n \t\tif (oid_object_info(the_repository, &e->idx.oid, &size) < 0)\n \t\t\tdie(_(\"unable to get size of %s\"),\n \t\t\t    oid_to_hex(&e->idx.oid));\n-\t\tread_unlock();\n+\t\tpacking_data_unlock(&to_pack);\n \t\treturn size;\n \t}\n \n@@ -2004,7 +2000,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,\n \tif (!p)\n \t\tBUG(\"when e->type is a delta, it must belong to a pack\");\n \n-\tread_lock();\n+\tpacking_data_lock(&to_pack);\n \tw_curs = NULL;\n \tbuf = use_pack(p, &w_curs, e->in_pack_offset, &avail);\n \tused = unpack_object_header_buffer(buf, avail, &type, &size);\n@@ -2013,7 +2009,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,\n \t\t    oid_to_hex(&e->idx.oid));\n \n \tunuse_pack(&w_curs);\n-\tread_unlock();\n+\tpacking_data_unlock(&to_pack);\n \treturn size;\n }\n \n@@ -2075,9 +2071,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,\n \n \t/* Load data if not already done */\n \tif (!trg->data) {\n-\t\tread_lock();\n+\t\tpacking_data_lock(&to_pack);\n \t\ttrg->data = read_object_file(&trg_entry->idx.oid, &type, &sz);\n-\t\tread_unlock();\n+\t\tpacking_data_unlock(&to_pack);\n \t\tif (!trg->data)\n \t\t\tdie(_(\"object %s cannot be read\"),\n \t\t\t    oid_to_hex(&trg_entry->idx.oid));\n@@ -2088,9 +2084,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,\n \t\t*mem_usage += sz;\n \t}\n \tif (!src->data) {\n-\t\tread_lock();\n+\t\tpacking_data_lock(&to_pack);\n \t\tsrc->data = read_object_file(&src_entry->idx.oid, &type, &sz);\n-\t\tread_unlock();\n+\t\tpacking_data_unlock(&to_pack);\n \t\tif (!src->data) {\n \t\t\tif (src_entry->preferred_base) {\n \t\t\t\tstatic int warned = 0;\n@@ -2336,9 +2332,9 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,\n \n static void try_to_free_from_threads(size_t size)\n {\n-\tread_lock();\n+\tpacking_data_lock(&to_pack);\n \trelease_pack_memory(size);\n-\tread_unlock();\n+\tpacking_data_unlock(&to_pack);\n }\n \n static try_to_free_t old_try_to_free_routine;\ndiff --git a/pack-objects.c b/pack-objects.c\nindex 3554c43ac..a1dc5eb72 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -148,10 +148,7 @@ void prepare_packing_data(struct packing_data *pdata)\n \t\t\t\t\t     1U << OE_SIZE_BITS);\n \tpdata->oe_delta_size_limit = git_env_ulong(\"GIT_TEST_OE_DELTA_SIZE\",\n \t\t\t\t\t\t   1UL << OE_DELTA_SIZE_BITS);\n-#ifndef NO_PTHREADS\n-\tpthread_mutex_init(&pdata->lock, NULL);\n-\tinit_recursive_mutex(&pdata->read_lock);\n-#endif\n+\tinit_recursive_mutex(&pdata->odb_lock);\n }\n \n struct object_entry *packlist_alloc(struct packing_data *pdata,\ndiff --git a/pack-objects.h b/pack-objects.h\nindex 0a038e3bc..1667cbad8 100644\n--- a/pack-objects.h\n+++ b/pack-objects.h\n@@ -145,8 +145,11 @@ struct packing_data {\n \tstruct packed_git **in_pack_by_idx;\n \tstruct packed_git **in_pack;\n \n-\tpthread_mutex_t lock;\n-\tpthread_mutex_t read_lock;\n+\t/*\n+\t * During packing with multiple threads, protect the in-core\n+\t * object database from concurrent accesses.\n+\t */\n+\tpthread_mutex_t odb_lock;\n \n \t/*\n \t * This list contains entries for bases which we know the other side\n@@ -166,22 +169,14 @@ struct packing_data {\n \n void prepare_packing_data(struct packing_data *pdata);\n \n+/* Protect access to object database */\n static inline void packing_data_lock(struct packing_data *pdata)\n {\n-\tpthread_mutex_lock(&pdata->lock);\n+\tpthread_mutex_lock(&pdata->odb_lock);\n }\n static inline void packing_data_unlock(struct packing_data *pdata)\n {\n-\tpthread_mutex_unlock(&pdata->lock);\n-}\n-\n-static inline void packing_data_read_lock(struct packing_data *pdata)\n-{\n-\tpthread_mutex_lock(&pdata->read_lock);\n-}\n-static inline void packing_data_read_unlock(struct packing_data *pdata)\n-{\n-\tpthread_mutex_unlock(&pdata->read_lock);\n+\tpthread_mutex_unlock(&pdata->odb_lock);\n }\n \n struct object_entry *packlist_alloc(struct packing_data *pdata,\n-- \n2.20.1.windows.1\n\n"}]}