Hi Patrick,
On Thu, 17 Jan 2019, Patrick Hogg 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 splitting off the read mutex initialization from
> init_threaded_search. Instead initialize (and clean up) the read
> mutex in cmd_pack_objects.
Very good explanation.
Two suggestions:
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 411aefd68..9084bef02 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2381,22 +2381,30 @@ 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);
> old_try_to_free_routine = set_try_to_free_routine(try_to_free_from_threads);
> }
>
> +static void init_read_mutex(void)
> +{
> + init_recursive_mutex(&read_mutex);
> +}
> +
> 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);
> }
>
> +static void cleanup_read_mutex(void)
> +{
> + pthread_mutex_destroy(&read_mutex);
> +}
> +
> static void *threaded_find_deltas(void *arg)
> {
> struct thread_params *me = arg;
> @@ -3319,6 +3327,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
> OPT_END(),
> };
>
> + init_read_mutex();
As the `read_mutex` is file-local, and as it really is only initialized
(or for that matter, cleaned up) in *one* spot, why not just spell out the
one-liner instead of introducing two new functions?
> +
> if (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS))
> BUG("too many dfs states, increase OE_DFS_STATE_BITS");
>
> @@ -3495,5 +3505,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
> _("Total %"PRIu32" (delta %"PRIu32"),"
> " reused %"PRIu32" (delta %"PRIu32")"),
> written, written_delta, reused, reused_delta);
> +
> + cleanup_read_mutex();
This misses one early `return`:
if (non_empty && !nr_result)
return 0;
I'd still suggest to just write out
if (non_empty && !nr_result) {
pthread_mutex_destroy(&read_mutex);
return 0;
}
even if there are now two call sites.
Ciao,
Johannes
> return 0;
> }
> --
> 2.20.1.windows.1
>
>
