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

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
To: Jeff King <peff@peff.net>
Cc: "Jonathan Tan" <jonathantanmy@google.com>,
	git <git@vger.kernel.org>,
	"Christian Couder" <christian.couder@gmail.com>,
	"Оля Тележная" <olyatelezhnaya@gmail.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Jonathan Nieder" <jrnieder@gmail.com>,
	"Stefan Beller" <stefanbeller@gmail.com>
Subject: Re: [PATCH v2 05/11] object-store: allow threaded access to object reading
Date: Thu, 19 Dec 2019 19:27:42 -0300	[thread overview]
Message-ID: <CAHd-oW5qT5LmUd6GTL=O+-yXPmq5Uy9gk3ohL_2r+_K+6UJS3Q@mail.gmail.com> (raw)
In-Reply-To: <20191115041215.GB21654@sigill.intra.peff.net>

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