Re: [PATCH 4/4] commit-graph: close descriptors after mmap

[Derrick Stolee <stolee@gmail.com>]

On 4/23/2020 6:04 PM, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> 
>> From: Jeff King <peff@peff.net>
>>
>> We don't ever refer to the descriptor after mmap-ing it. And keeping it
>> open means we can run out of descriptors in degenerate cases (e.g.,
>> thousands of split chain files). Let's close it as soon as possible.
> 
> Yikes.  
> 
> Sorry, I should have looked at the use of mmap in this topioc more
> carefully when we queued the series.  It is an easy mistake to make
> by anybody new to the API to leave it open while the region is in
> use.

You are right. I was new when first contributing the commit-graph. It
was also easier to miss because we only had one commit-graph open at
the time. Adding in the incremental file format led to multiple file
descriptors being open.

However, this idea of closing a descriptor after an mmap is new to
me. So I thought about other situations where I made the same mistake.
Please see the patch below.

> With this fix, with or without the other topics still in flight, I
> do not think no code touches graph_fd.  Should we remove the
> graph_fd field from the structure as well?

I agree that this should be done.

Thanks,
-Stolee

-->8--

From: Derrick Stolee <dstolee@microsoft.com>
Date: Fri, 24 Apr 2020 13:11:13 +0000
Subject: [PATCH] multi-pack-index: close file descriptor after mmap

We recently discovered that the commit-graph was not closing its
file descriptor after memory-mapping the file contents. After this
mmap() succeeds, there is no need to keep the file descriptor open.
In fact, there is signficant reason to close it so we do not run
out of descriptors.

This is entirely my fault for not knowing that we can have an open
mmap while also closing the descriptor. Some could blame this on
being a new contributor when the series was first submitted, but
even years later this is still new information to me.

That made me realize that I used the same pattern when opening a
multi-pack-index. Since this file is not (yet) incremental, there
will be at most one descriptor open for this reason. It is still
worth fixing, especially if we extend the format to be incremental
like the commit-graph.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 midx.c | 4 +---
 midx.h | 2 --
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/midx.c b/midx.c
index 1527e464a7..60d30e873b 100644
--- a/midx.c
+++ b/midx.c
@@ -72,9 +72,9 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
 	FREE_AND_NULL(midx_name);
 
 	midx_map = xmmap(NULL, midx_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	close(fd);
 
 	FLEX_ALLOC_STR(m, object_dir, object_dir);
-	m->fd = fd;
 	m->data = midx_map;
 	m->data_len = midx_size;
 	m->local = local;
@@ -190,8 +190,6 @@ void close_midx(struct multi_pack_index *m)
 		return;
 
 	munmap((unsigned char *)m->data, m->data_len);
-	close(m->fd);
-	m->fd = -1;
 
 	for (i = 0; i < m->num_packs; i++) {
 		if (m->packs[i])
diff --git a/midx.h b/midx.h
index e6fa356b5c..b18cf53bc4 100644
--- a/midx.h
+++ b/midx.h
@@ -12,8 +12,6 @@ struct repository;
 struct multi_pack_index {
 	struct multi_pack_index *next;
 
-	int fd;
-
 	const unsigned char *data;
 	size_t data_len;
 
-- 
2.26.2