Re: [PATCH v2 05/11] object-store: allow threaded access to object reading

[Jonathan Tan <jonathantanmy@google.com>]

> @@ -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.