Duy Nguyen <pclouds@xxxxxxxxx> writes:
> On Thu, Jan 24, 2019 at 8:06 AM Patrick Hogg <phogg@xxxxxxxxxxxx> wrote:
>> diff --git a/pack-objects.h b/pack-objects.h
>> index 0a038e3bc..dc869f26c 100644
>> --- a/pack-objects.h
>> +++ b/pack-objects.h
>> @@ -146,7 +146,6 @@ struct packing_data {
>> struct packed_git **in_pack;
>>
>> pthread_mutex_t lock;
>> - pthread_mutex_t read_lock;
>
> "lock" without any comments in this struct, to me, implies that it
> protects access to this struct alone. But since you're using it as
> "read lock" (aka access to object database), I think you should add a
> comment here clarify the new role of "lock" variable.
Sounds sensible. How about squashing something like this in?
Some older part of this file still tries to hide the reliance on the
global variable "to_pack", but newer code refers to it already, and
I think it no longer is buying us much.
builtin/pack-objects.c | 24 ++++++++++--------------
pack-objects.c | 2 +-
pack-objects.h | 11 ++++++++---
3 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 5439b434c6..cfef48217f 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 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;
#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) {
- pack_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));
- pack_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");
- pack_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);
- pack_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) {
- pack_lock();
+ packing_data_lock(&to_pack);
trg->data = read_object_file(&trg_entry->idx.oid, &type, &sz);
- pack_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) {
- pack_lock();
+ packing_data_lock(&to_pack);
src->data = read_object_file(&src_entry->idx.oid, &type, &sz);
- pack_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)
{
- pack_lock();
+ packing_data_lock(&to_pack);
release_pack_memory(size);
- pack_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 6f32a7ba04..a1dc5eb726 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -148,7 +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);
- init_recursive_mutex(&pdata->lock);
+ 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 dc869f26c2..a0c21959b2 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -145,7 +145,11 @@ struct packing_data {
struct packed_git **in_pack_by_idx;
struct packed_git **in_pack;
- pthread_mutex_t 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
@@ -165,13 +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);
+ pthread_mutex_unlock(&pdata->odb_lock);
}
struct object_entry *packlist_alloc(struct packing_data *pdata,
