On Thu, Jan 24, 2019 at 8:06 AM Patrick Hogg <phogg@xxxxxxxxxxxx> wrote: > > 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. Obviously correct. Reviewed-by: Duy Nguyen <pclouds@xxxxxxxxx> > > Signed-off-by: Patrick Hogg <phogg@xxxxxxxxxxxx> > --- > 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 > -- Duy