On Mon, Nov 11, 2019 at 06:54:18PM -0800, Jonathan Tan wrote:
> > @@ -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.
Yeah, I don't think the code above is safe. The map window can be
modified by other threads.
> 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.
I think you could put a reader-writer lock into each window. The code
here would take the reader lock, and multiple readers could use it at
the same time. Any time the window needs to be shifted, resized, or
discarded, that code would take the writer lock, waiting for (and then
blocking out) any readers.
A pthread_rwlock would work, but it would be the first use in Git. I
think we'd need to find an equivalent for compat/win32/pthread.h.
-Peff
