Re: [PATCH v3 1/2] pack-objects: move read mutex to packing_data struct

>
> 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 <phogg@novamoon.net>
> ---
>  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
>