Hi, Peff and Jonathan On Wed, Nov 13, 2019 at 2:20 AM Jeff King <peff@xxxxxxxx> wrote: > > On Mon, Nov 11, 2019 at 06:54:18PM -0800, Jonathan Tan wrote: [...] > > 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. Thank you for spotting this issue! > > 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. Great idea, I'll work on that. Thanks! About the other parallel inflation calls on loose objects at unpack_loose_short_header(), unpack_loose_header_to_strbuf() and unpack_loose_rest(): could they suffer from a similar race problem? FWIU, the inflation input used in these cases comes from map_loose_object(), and it's not referenced outside this scope. So I think there's no risk of one thread munmapping the object file while other is inflating it. Is that right? > 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. These[1][2] seems to be the equivalent options on Windows. I'll have to read these docs more carefully, but [2] seems to be more interesting in terms of speed. Also, the extra features of [1] are not really needed for our use case here. [1]: https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/reader-writer-spin-locks [2]: https://docs.microsoft.com/en-us/windows/win32/sync/slim-reader-writer--srw--locks
