Re: [PATCH v2 05/11] object-store: allow threaded access to object reading
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Nov 12, 2019, 02:54 UTC
- Message-ID
- <20191112025418.254880-1-jonathantanmy@google.com>
- In-Reply-To
- <4c5652ab34f0989856aba919ca84b2b091dcad98.1569808052.git.matheus.bernardino@usp.br>
Show 10 quoted lines
> @@ -1580,7 +1585,9 @@ static void *unpack_compressed_entry(struct packed_git *p,
> do {
> in = use_pack(p, w_curs, curpos, &stream.avail_in);
> stream.next_in = in;
> + obj_read_unlock();
> st = git_inflate(&stream, Z_FINISH);
> + obj_read_lock();
> if (!stream.avail_out)
> break; /* the payload is larger than it should be */
> curpos += stream.next_in - in;As I see it, the main purpose of this patch set is to move the mutex guarding object reading from builtin/grep.c (grep_read_mutex) to object-store.h (obj_read_mutex), so that we can add "holes" (non-mutex sections) such as the one quoted above, in order that zlib inflation can happen outside the mutex.
My concern is that the presence of these "holes" make object reading non-thread-safe, defeating the purpose of obj_read_mutex. In particular, the section quoted above assumes that the window section returned by use_pack() is still valid throughout the inflation, but that window could have been invalidated by things like an excess of windows open, reprepare_packed_git(), etc.
I thought of this for a while but couldn't think of a good solution. If we introduced a reference counting mechanism into Git, that would allow us to hold the window open outside the mutex, but things like reprepare_packed_git() would still be difficult.
If there's a good solution that only works for unpack_compressed_entry() and not the other parts that also inflate, that might be sufficient (we can inflate outside mutex just for this function, and inflate inside mutex for the rest) - we would have to benchmark to be sure, but I think that Matheus's main use case concerns grepping over a repo with a packfile, not loose objects.