Atneya Nair <atneya@xxxxxxxxxx> writes:
> To enable parallel update of the read cache for submodules,
> ce_compare_gitlink must be thread safe (for different objects).
>
> Remove string interning in do_config_from (called from
> repo_submodule_init) and add locking around accessing the ref_store_map.
This step does two independent things, even though they may have
dependencies, i.e., for one to be a solution for the problem it is
tackling, the other may have to be there already. E.g., even after
calls to ce_compare_gitlink() get serialized via a mutex, it may for
some reason not work without giving each kvi.filename its own copy
[*], and if that is the case, you may need to have the "stop
interning" step in a single patch with its own justification, and
then "have mutex around ref_store calls" patch has to come after it.
Side note: I do not know if that is the case myself. I didn't
write this commit, you did. The above is just a sample to
illustrate the expected level of depth to explain your thinking
in the log message.
Or if these two things must happen at the same time, please explain
in the proposed log message why they have to happen in the same
commit. The two paragraphs you wrote there don't explain that, so I
am assuming that it is not the case.
The use of strintern() comes originally from 3df8fd62 (add line
number and file name info to `config_set`, 2014-08-07) by Tanay
Abhra <tanayabh@xxxxxxxxx>, and survived a handful of changes
809d8680 (config: pass ctx with config files, 2023-06-28)
a798a56c (config.c: plumb the_reader through callbacks, 2023-03-28)
c97f3ed2 (config.c: plumb config_source through static fns, 2023-03-28)
all of which were done by Glen Choo <glencbz@xxxxxxxxx>, so they may
know how safe the change on the config side would be (I still do
not understand why you'd want to do this in the first place, though,
especially if you are protecting the callsites with mutex).
I also think Emily's (who you already have on the "CC:" line) group
wants to libify the config machinery and suspect they may still be
making changes to the code, so you may want to coordinate with them
to avoid duplicated work and overlapping changes.
> Signed-off-by: Atneya Nair <atneya@xxxxxxxxxx>
> ---
>
> Notes:
> Chasing down thread unsafe code was done using tsan.
Very nice to know.
> config.c | 3 ++-
> config.h | 2 +-
> refs.c | 9 +++++++++
> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/config.c b/config.c
> index 3cfeb3d8bd..d7f73d8745 100644
> --- a/config.c
> +++ b/config.c
> @@ -1017,7 +1017,7 @@ static void kvi_from_source(struct config_source *cs,
> enum config_scope scope,
> struct key_value_info *out)
> {
> - out->filename = strintern(cs->name);
> + out->filename = strdup(cs->name);
> out->origin_type = cs->origin_type;
> out->linenr = cs->linenr;
> out->scope = scope;
> @@ -1857,6 +1857,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn,
>
> strbuf_release(&top->value);
> strbuf_release(&top->var);
> + free(kvi.filename);
>
> return ret;
> }
> diff --git a/config.h b/config.h
> index 5dba984f77..b78f1b6667 100644
> --- a/config.h
> +++ b/config.h
> @@ -118,7 +118,7 @@ struct config_options {
>
> /* Config source metadata for a given config key-value pair */
> struct key_value_info {
> - const char *filename;
> + char *filename;
> int linenr;
> enum config_origin_type origin_type;
> enum config_scope scope;
> diff --git a/refs.c b/refs.c
> index c633abf284..cce8a31b22 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2126,6 +2126,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
> size_t len;
> struct repository *subrepo;
>
> + // TODO is this locking tolerable, and/or can we get any finer
> + static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
> +
> if (!submodule)
> return NULL;
>
> @@ -2139,7 +2142,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
> /* We need to strip off one or more trailing slashes */
> submodule = to_free = xmemdupz(submodule, len);
>
> + pthread_mutex_lock(&lock);
> refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
> + pthread_mutex_unlock(&lock);
> if (refs)
> goto done;
>
> @@ -2162,10 +2167,14 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
> free(subrepo);
> goto done;
> }
> +
> + pthread_mutex_lock(&lock);
> + // TODO maybe lock this separately
> refs = ref_store_init(subrepo, submodule_sb.buf,
> REF_STORE_READ | REF_STORE_ODB);
> register_ref_store_map(&submodule_ref_stores, "submodule",
> refs, submodule);
> + pthread_mutex_unlock(&lock);
>
> done:
> strbuf_release(&submodule_sb);
