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

Hi, Peff and Jonathan

Sorry for the delay in re-rolling this series, it was a very busy end of semester. But I finally completed my degree and had time to get back to it :)

I tried the rwlock approach, but there were some subtle difficulties. For example, we should hold the lock in write mode when free()-ing the window, and thus, the lock couldn't be declared inside the struct pack_window.

Also, it seemed that protecting the window reading at git_inflate() wouldn't be enough: suppose a thread has just read from a window in git_inflate() (holding the rwlock) and is now waiting for the obj_read_mutex to continue its object reading operation. If another thread (that already has the obj_read_mutex) acquires the rwlock in sequence, it could free() the said window. It might not sound like a problem since the first thread has already finished reading from it. But since a pointer to the window would still be in the first thread's stack as a window cursor, it could be later passed down to use_pack() leading to a segfault. I couldn't come up with a solution for this yet.

However, re-inspecting the code, it seemed to me that we might already have a thread-safe mechanism. The window disposal operations (at close_pack_windows() and unuse_one_window()) are only performed if window.inuse_cnt == 0. So as a thread which reads from the window will also previously increment its inuse_cnt, wouldn't the reading operation be already protected?

Another concern would be close_pack_fd(), which can close packs even with in-use windows. However, as the mmap docs[1] says: "closing the file descriptor does not unmap the region".

Finally, we also considered reprepare_packed_git() as a possible conflicting operation. But the function called by it to handle packfile opening, prepare_pack(), won't reopen already available packs. Therefore, IIUC, it will leave the opened windows intact.

So, aren't perhaps the window readings performed outside the obj_read_mutex critical section already thread-safe?

Thanks, Matheus

[1]: https://linux.die.net/man/2/mmap