Re: [PATCH 2/4] map_loose_object_1: attempt to handle ENOMEM from mmap

From: Jeff King <peff@peff.net>
Date: Jun 30, 2021, 02:41 UTC
Message-ID: <YNvZ3lA6J8DIx2r4@coredump.intra.peff.net>
In-Reply-To: <20210629081108.28657-3-e@80x24.org>

On Tue, Jun 29, 2021 at 08:11:06AM +0000, Eric Wong wrote:

Show 8 quoted lines

> This benefits unprivileged users who lack permissions to raise
> the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource
> limit.
> 
> Multi-threaded callers to map_loose_object_1 (e.g. "git grep")
> appear to guard against thread-safety problems.  Other
> multi-core aware pieces of code (e.g. parallel-checkout) uses
> child processes

This one makes me slightly more nervous than the last, because we don't know if somebody higher up the call stack might be expecting to use that window again.

I _think_ this one is OK, because we would never read a loose object in the process of reading a packed one. I thought we might in some rare cases with corruption (e.g., B is a delta against A; we fail to read A because it's corrupt, so we look elsewhere for a copy). I don't think the current code is quite that clever, though. If B were corrupted, we'd look elsewhere for a duplicate, but that logic doesn't apply in the middle of a delta resolution (even though there are corruption cases it would help recover from).

So I don't think this is introducing any bugs. But it worries me a bit that it creates an opportunity for a subtle and hard-to-trigger situation if we do change seemingly-unrelated code.

But looking more carefully at unuse_one_window(), I think the worst case is a slight performance drop, as we unmap the window and then re-map when somebody higher up the call-stack needs it again. My real worry was that we could trigger a case where somebody was holding onto a pointer to the bytes. But I think use_pack() will mark those bytes via inuse_cnt, and then unuse_one_window() will never drop a window that has a non-zero inuse_count.

So in that sense all of your patches should be correct without thinking too hard no them. Sure, a performance drop from having to re-map the window later isn't great, but if the alternative in each case is calling die(), it's a strict improvement.

-Peff