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

[Matheus Tavares Bernardino <matheus.bernardino@usp.br>]

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